Github user cestella commented on the issue:
https://github.com/apache/incubator-metron/pull/395
@mattf-horton @mmiklavc First off, thanks for the perspective, both of
you. While I think we should investigate your suggestion, @mattf-horton , it
appears on the face of it quite dependent upon implementation decisions and
details inside of Storm. That immediately gives me pause. That being said,
I'll repeat that I think we should consider it strongly.
I do agree with @mattf-horton that pulling out the `ZkConfigurationManager`
here would be substantially more work if you consider the impact to our tests
and the increased amount of testing that we would have to add to this PR. I
think, as @mmiklavc suggested, it's a separate PR. I would prefer to get this
really good work by @nickwallen in, so maybe I can offer another compromise
position to go with @mattf-horton 's one earlier.
I'd like to propose another compromise. The main concern that I have is
leaving an second abstraction in place for managing configuration from
zookeeper. Given that, what do you think of the following:
* Remove the `ConfigurationManager` abstraction from this PR
* Since it is used only in one class now, directly use the
`org.apache.curator.framework.recipes.cache.NodeCache` in `ProfilerSplitterBolt`
* Create a follow-on JIRA and make a comment with a TODO suggesting we
replace this with the appropriate abstraction (referencing the JIRA).
This would remove the possibility of a fork and be substantially less
work/impact.
Thoughts?
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
