metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From merrimanr <...@git.apache.org>
Subject [GitHub] metron issue #717: METRON-1134: Allow parser command line options to be spec...
Date Tue, 29 Aug 2017 20:42:42 GMT
Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/717
  
    I tested this in full dev and everything worked as advertised.  The only major issue I
see is that the num workers property is missing from SensorParserConfig.  I feel like this
is one of the most important settings for a topology.  You could set it with "topology.workers"
in the "stormConfig" property but it's not obvious and I think this setting should be a first
class citizen like the others.
    
    For me, the use of the ValueSupplier functional interface made it more difficult to understand
when reviewing.  The code seemed more verbose and complex and I wasn't able to work out the
benefit of using it in this case.  It's is a very minor style issue and is just my (probably
worthless) opinion.  I would not hold up the PR over something like this, just some honest
feedback.
    
    Awesome work.  If we can promote num workers to a getter method in SensorParserConfig
then I'm confident giving a +1.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message