Github user mattf-horton commented on the issue:
https://github.com/apache/incubator-metron/pull/395
Regarding ZkConfigurationManager and the above discussion thread:
This is a complex situation, but my best advice is as follows:
1. It is definitely worthwhile to refactor a CuratorZkConfigManager class
out of ConfiguredBolt. In fact, when we do, we really should propose the Storm
team adopt it; it would be tremendously valuable for lots of Storm
applications. HOWEVER, this is a significant task in its own right and more
than a couple days' work. I believe there is a much simpler solution for the
short term, that either avoids or mostly avoids the split implementation
problem (depending on a Storm detail that I wasn't able to determine), so I
don't recommend we make Nick do it as part of this task.
2. I agree with @merrimanr and @cestella that we should not lose the
established functionality of ConfiguredBolt for managing
ProfilerConfigurations, nor create a new config management and caching
mechanism, even with good intentions about putting it all back together in the
future.
3. After analyzing what goes into the class hierarchies, I see that:
a) The way BaseWindowedBolt manages its windowConfiguration is
qualitatively different from the Curated Zk based management used by
ConfiguredBolt. It would be hard to merge them, and that's what is causing the
current problem.
b) But it isn't necessary, because the config management built into
BaseWindowedBolt is just a distraction.
What's necessary to use Storm Windowing is that:
- the Bolt implements IWindowedBolt interface,
- a certain set of 7 parameters (defined by example in BaseWindowedBolt) is
presented to Storm when it queries the Bolt's getComponentConfiguration()
method,
- and the Storm topology correctly identifies the Bolt as being a Windowed
Bolt.
The only question in my mind is the last bullet. Does Storm identify the
Bolt as Windowed just because it presents the windowing parameters in
getComponentConfiguration() ? Or does it do class reflection on the Bolt? If
it uses reflection, is it looking for IWindowedBolt or for BaseWindowedBolt ?
It is likely that one of the first two is sufficient to identify the Bolt
as Windowed. If so, there's a very easy solution to all this. Define:
```
public abstract class ConfiguredWindowedBolt<CONFIG_T extends
Configurations>
extends ConfiguredBolt<CONFIG_T> implements IWindowedBolt
{with only a few lines of content needed}
and
public abstract class ConfiguredProfilerBolt
extends ConfiguredWindowedBolt<ProfilerConfigurations>
```
This compiles on the master codeline; I can share my straw man if desired.
Then change `getComponentConfiguration()` in ProfileBuilderBolt, to return
the 7 windowing-related config parameters instead of
TOPOLOGY_TICK_TUPLE_FREQ_SECS. Other than changes to add the windowing params
to the ProfilerConfigurations on a per-bolt basis, the current config
management code in ConfiguredProfilerBolt and ConfiguredBolt can be used just
as is.
If Storm actually requires the Bolt to be of type BaseWindowedBolt, then it
is a little uglier, but still doable: Copy the methods of ConfiguredBolt into
a new class that extends BaseWindowedBolt, simply overriding any conflicting
methods already in BaseWindowedBolt. We have to duplicate the code, but it can
be copied verbatim, thereby giving much higher reliability than new code.
Again, there's no need to try to use the configuration management code
built into BaseWindowedBolt; only what gets returned in
`getComponentConfiguration()` is important.
I don't expect either of these solutions to give great joy, but it is
workable and can be done reliably. We don't need to refactor ConfiguredBolt,
and its curated config management can be used whole.
---
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.
---
