Use a factory to create VirtualSpout instances #71
Conversation
| // it's our firehose. | ||
| fireHoseIdentifier, | ||
| getSpoutConfig(), | ||
| topologyContext, |
There was a problem hiding this comment.
I like this approach. Do we need to store a copy of topologyContext anymore?
There was a problem hiding this comment.
Probably not. I don’t think we use it. Should we just drop it?
There was a problem hiding this comment.
It's used to construct ConsumerPeerContext, which should maybe just be moved to a constructor parameter? Not sure. I started stripping topologyContext out, I'll stick it in a branch in case we want to revisit but I don't think we should sweat it right now.
There was a problem hiding this comment.
oh its just used in open right, i just meant dropping saving the reference in the class beyond the scope of where its used. Not sure it matters much tho
| * Handy in things like {@link com.salesforce.storm.spout.dynamic.handler.SpoutHandler} where we want to abstract away particulars, such | ||
| * as the things passed down from {@link DynamicSpout} such as the {@link MetricsRecorder} as an example. | ||
| */ | ||
| public class VirtualSpoutFactory implements DelegateSpoutFactory { |
There was a problem hiding this comment.
Added, thanks for keeping me honest.
IMO this is related to #70 as you can tell by some of the naming weirdness.
FactoryManagerdoesn't have any facilities for passing arguments to a constructor, which is why it was not used to create theVirtualSpoutFactory.I have mixed feelings about the generics in
DelegateSpoutFactory, in the end if this is not done we wind up casting, so I think this is a better option honestly.Note that right now the
FilterChainis part ofVirtualSpoutand not ofDelegateSpout, I think this is worth calling out because it is the primary reason why someone would want to directly type againstVirtualSpouttoday. Perhaps we should reconsider this, I'm open to this - we just need to make a decision that theFilterChainis a first class citizen in the interface and that we expect implementations to utilize this.