ppat / storm-rabbitmq

A library of tools for interacting with RabbitMQ from Storm.
MIT License
126 stars 77 forks source link

Enhanced declareOutputFields #22

Closed bdgould closed 9 years ago

bdgould commented 9 years ago

Only declaring output fields when the set is non-null and non-empty now.

ppat commented 9 years ago

When the fields are declared, we still need to emit them in the execute method, right? That was your intention? If so, currently nothing is being emitted even when the output fields are defined.

bdgould commented 9 years ago

Actually, the AbstractTupleToMessage was just going to be a helper so that developers who didn't want to emit anything (just drain from the stream) could just extend that to do the actual mapping of tuple to a message object. I figured any implementation of the TupleToMessage that did want to emit tuples could do so in it's produceMessage(...) method. That way a developer could use the bolt as a simple drain on the stream, or if they wanted to, could have the OutputCollector available to them in the TupleToMessage so that they could emit data.

ppat commented 9 years ago

Hmmm... I'd like to give TupleToMessage the singular responsibility of transforming a tuple to a message and not conflate with the additional responsibility of doing an emit. I might merge this in but I think I will refactor the optional emit (and the associate the outputFields) into the bolt.

ppat commented 9 years ago

Do you have a use case where you actually use the RabbitMQBolt as a sink and then emit a tuple afterwards? If so, I'd like to hear more details about it. That might help us come with a better design (or if not, get rid of the emit all together).

bdgould commented 9 years ago

After thinking it over a little more I agree. Makes more sense to just have the bolt simply act as a sink. If a user wanted to do further processing on the stream they could always subscribe another bolt next to the RabbitMQBolt. It makes more sense to just never emit tuples.

bdgould commented 9 years ago

Would you like me to do another request? Or do you want to merge and clear out the declare parts yourself?

bdgould commented 9 years ago

I just made the changes, let me know if that makes more sense.

bdgould commented 9 years ago

I also included the ability to specify the streamId in the constructor of the RabbitMQSpout. I'm writing a topology now for work that this will help for. There are a couple inputs into the same bolt that behave slightly differently depending on the input source.

ppat commented 9 years ago

Great... thanks. Also take a look at the the Multi Stream Spout.

bdgould commented 9 years ago

Ahh, that's true. I didn't think about just always specifying a single stream from the MultiStreamSplitter. I suppose I could do that, though having the option to do either isn't bad, right?