Github user cestella commented on the issue:
https://github.com/apache/metron/pull/737
+1 by inspection. This looks good to me. Great job!
---
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/737
Ok I reverted the SensorParserConfig class so that defaults are not
provided for numWorkers and numAckers. I also added help text to make it clear
to the user what happens when these are not set.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/737
Wait, I'm confused, the current behavior is to default to the storm config.
Why did you pull that into the SensorParserConfig?
The issue with what you've done here is that when you save
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/737
I moved the default values to the storm config and changed getNumWorkers
and getNumAckers to return the values in the storm config if
numWorkers/numAckers is null. Is this what you were thinking?
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/737
So, I do believe the default values for num workers and ackers should be
taken from the storm config if unspecified by us. Providing defaults when
storm already has defaults for these properties
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/737
Changing numWorkers and numAckers default values from null to 1 had the
unintended consequences of breaking the ParserTopologyCliTest. This is because
they can no longer be overriden by setting
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/737
Just pushed out a commit that addresses @ottobackwards comments and fixes
the NUM_WORKERS and NUM_ACKERS issue. I also added a warning that appears when
a storm setting changes, telling the user
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/737
Thank you for the feedback @nickwallen.
On points 1-3 and 5, these could be applied to any panel in the management
UI. They all work the same. My opinion is that correct design in this
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/737
@merrimanr This will be uber useful. Here are my initial reactions
focusing on usability. I am totally open to knocking some of these out as
separate PRs, should that be a better approach.
