You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by HeartSaVioR <gi...@git.apache.org> on 2017/02/28 09:37:53 UTC
[GitHub] storm pull request #1978: STORM-2387 Handle tick tuples properly for Bolts i...
GitHub user HeartSaVioR opened a pull request:
https://github.com/apache/storm/pull/1978
STORM-2387 Handle tick tuples properly for Bolts in external modules
* also remove ack for tick tuples since it's not necessary
I also fixed some mixing indentations. We should introduce code style soon.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HeartSaVioR/storm STORM-2387
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1978.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1978
----
commit 6e4b42e3af885595d131bd10080078c6a56a3730
Author: Jungtaek Lim <ka...@gmail.com>
Date: 2017-02-28T08:19:58Z
STORM-2387 Handle tick tuples properly for Bolts in external modules
* also remove ack for tick tuples since it's not necessary
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1978: STORM-2387 Handle tick tuples properly for Bolts i...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1978#discussion_r103610033
--- Diff: external/storm-druid/src/main/java/org/apache/storm/druid/bolt/DruidBeamBolt.java ---
@@ -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)) {
--- End diff --
@HeartSaVioR I am fine with `TickTupleAwareBaseRichBolt` unless a better name is found.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1978: STORM-2387 Handle tick tuples properly for Bolts i...
Posted by priyank5485 <gi...@git.apache.org>.
Github user priyank5485 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1978#discussion_r103527554
--- Diff: external/storm-druid/src/main/java/org/apache/storm/druid/bolt/DruidBeamBolt.java ---
@@ -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)) {
--- End diff --
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?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1978: STORM-2387 Handle tick tuples properly for Bolts i...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on a diff in the pull request:
https://github.com/apache/storm/pull/1978#discussion_r103422898
--- Diff: external/storm-druid/src/main/java/org/apache/storm/druid/bolt/DruidBeamBolt.java ---
@@ -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)) {
--- End diff --
Most of these connector bolts extend `BaseRichBolt`. It may be good to have a class extending BaseRichBolt and handle ticktuples. This class can be used across other connector bolts instead of handling them in respective connector's base classes.
```
// you 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) {
}
}
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1978: STORM-2387 Handle tick tuples properly for Bolts i...
Posted by dossett <gi...@git.apache.org>.
Github user dossett commented on a diff in the pull request:
https://github.com/apache/storm/pull/1978#discussion_r103453575
--- Diff: external/storm-druid/src/main/java/org/apache/storm/druid/bolt/DruidBeamBolt.java ---
@@ -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)) {
--- End diff --
@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
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1978: STORM-2387 Handle tick tuples properly for Bolts i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1978
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1978: STORM-2387 Handle tick tuples properly for Bolts in exter...
Posted by satishd <gi...@git.apache.org>.
Github user satishd commented on the issue:
https://github.com/apache/storm/pull/1978
@HeartSaVioR +1 LGTM
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1978: STORM-2387 Handle tick tuples properly for Bolts in exter...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1978
@satishd @priyank5485 Applied review comments.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1978: STORM-2387 Handle tick tuples properly for Bolts i...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/1978#discussion_r103552195
--- Diff: external/storm-druid/src/main/java/org/apache/storm/druid/bolt/DruidBeamBolt.java ---
@@ -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)) {
--- End diff --
@satishd
Make sense.
About class name I don't see good name for that. I thought `TickTupleAwareBaseRichBolt` inspired of 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?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---