search

salesforce / storm-dynamic-spout

A framework for building spouts for Apache Storm and a Kafka based spout for dynamically skipping messages to be processed later.
BSD 3-Clause "New" or "Revised" License
40 stars 13 forks source link

first pass at creating a concrete SpoutConfig class, lots of refactor… #82

Closed Crim closed 6 years ago

Crim commented 6 years ago

Requirements

Questions

Currently this is heavily based off of Kafka's configuration (see here: AbstractConfig and ConsumerConfig )

"SpoutConfig" is a generic Configuration holder.
"DynamicSpoutConfig" extends this supplying a ConfigDefinition, but has no logic beyond defining the ConfigDefinition and a pass thru constructor. "SidelineConfig" makes use of DynamicSpout's ConfigDefinition and appends its own settings to it, again no logic and pass thru constructors.

Not entirely sure how I feel about this model. Once we break up Kafka Consumer, how does that fit into this? Does it need to?

Even if we go a totally different direction from here, I think this branch gives us a good starting point with concrete classes to play around with. It was a lot more work than I was expecting to convert those maps into concrete classes :/

stanlemon commented 6 years ago

I'm not a big fan of having the docs so far removed from the config declaratives in their respective classes. This seems like a step back to me. I am a proponent of a concrete class and moving in that general direction though.

Help me understand the reason for splitting SpoutConfig and DynamicConfig, is the former just supposed to be a base class that can be used elsewhere?

Crim commented 6 years ago

So right now "SpoutConfig" is basically just a blank adhoc config object, allowing you to set a config definition, and get values out of it. I'd like to figure out a good way to satisfy bullet 3 at the top.

"Needs to allow user to define adhoc settings for any integrations."

stanlemon commented 6 years ago

@Crim what's the plan here? We're in merge conflict hell atm. Do we want to pursue this or abandon it for the time being?