STORM-2387 Handle tick tuples properly for Bolts in external modules #1978
Conversation
| @@ -76,30 +77,33 @@ public void prepare(Map stormConf, TopologyContext context, OutputCollector coll | |||
|
|
|||
| @Override | |||
| public void execute(final Tuple tuple) { | |||
| Future future = tranquilizer.send((druidEventMapper.getEvent(tuple))); | |||
| LOG.debug("Sent tuple : [{}]" , tuple); | |||
| if (TupleUtils.isTick(tuple)) { | |||
There was a problem hiding this comment.
Most of these connector bolts extend BaseRichBolt. It may be good to have a class extending BaseRichBolt and handle ticktuples which you have implemented for some of the connector's base classes. This class can be used across other connector bolts instead of handling them in respective connector's base classes. Respective class can be created for BaseBasicBolt if it is required.
// can have better class name if found any
public class BaseRichBoltWithTickTuples extends BaseRichBolt {
@Override
public void execute(final Tuple tuple) {
if(TupleUtils.isTick(tuple)) {
onTickTuple(tuple);
} else {
process(tuple);
}
}
protected void onTickTuple(final Tuple tuple) {
}
protected void onProcess(final Tuple tuple) {
}
}
There was a problem hiding this comment.
@satishd I agree in principle. I once experimented with that approach and what I found difficult was declaring the throws for each of those methods since bolts can generate all sorts of exceptions.
+1
There was a problem hiding this comment.
I agree with @satishd Right now, onTickTuple method is in abstract base classes of different connectors and not even used. Taking Satish's approach is a little more work since we will have to change the bolts that actually process the tick tuples to separate out onProcess and onTickTuple logic. But i think its worth. @dossett I did not exactly get the problem with exceptions. Can't we have a catch all and rethrow as a RuntimeException?
There was a problem hiding this comment.
@satishd
Make sense.
About class name I don't see good name for that. I thought TickTupleAwareBaseRichBolt inspired by Spring but the naming ~Aware is actually used for interfaces.
If we would want to make it public API (instead of using internally), it needs to have better class name.
@dossett
execute() also doesn't have throws definitions. If we follow @satishd execute() implementation, execute() doesn't care about exceptions and it's up to subclasses.
So handling exceptions for two separated methods (onTickTuple() and process()) could happen, but it doesn't hurt. It just makes some duplicate codes.
Do you have other considerations or point of view?
There was a problem hiding this comment.
@HeartSaVioR I am fine with TickTupleAwareBaseRichBolt unless a better name is found.
|
@satishd @priyank5485 Applied review comments. |
* introduce BaseTickTupleAwareRichBolt which makes logic branches for tick tuple and non-tick tuple * apply BaseTickTupleAwareRichBolt to Bolts in external modules * also remove ack for tick tuples since it's not necessary
|
@HeartSaVioR +1 LGTM |
I also fixed some mixing indentations. We should introduce code style soon.
For 1.x branch: #1979