You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by abhishekagarwal87 <gi...@git.apache.org> on 2016/01/19 17:17:11 UTC
[GitHub] storm pull request: STORM-1455: Do not reset the emittedOffset for...
GitHub user abhishekagarwal87 opened a pull request:
https://github.com/apache/storm/pull/1026
STORM-1455: Do not reset the emittedOffset for offsetOutOfRangeExceptions
There are two changes in this PR
- If there is a TopicOffsetOutOfRangeException, it can be due to an old failed tuple. We should not reset the state back to the beginning.
- Apart from the last completed offset, also emit the last emitted offset in the metric. latestEmittedOffset now points to the last emitted offset.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/abhishekagarwal87/storm kafka-spout
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1026.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 #1026
----
commit c1e8ec576a391c9431ec50eb8eb60c083417b4c7
Author: Abhishek Agarwal <ab...@inmobi.com>
Date: 2016-01-19T16:13:22Z
STORM-1455: Do not reset the emittedOffset for offsetOutOfRangeExceptions
----
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-175146222
Thanks @revans2 . Fixed the compilation error. I had missed some changes. To put these changes in 1.0 release, do I need to open another pull request?
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-174013629
Other then that it looks OK to me.
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-180221434
Forgot to leave comment, also backported to 1.x-branch.
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-176321658
can this be merged?
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-175148840
Typically you don't need a separate pull request. Just ask for it and we will cherry-pick it. If there are merge conflicts we may come back and ask for a separate pull request.
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-180207678
Merged to master, backporting to 1.x-branch...
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-172904085
@HeartSaVioR @revans2 can you review these changes?
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-180214818
Thanks @HeartSaVioR
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-175547593
Thank you. can you merge this into master and also select these commits for backport onto 1.0 release?
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1026
---
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: STORM-1455: Do not reset the emittedOffset for...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-180204666
@abhishekagarwal87
Sorry to review in late. Looks good to me, +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: STORM-1455: Do not reset the emittedOffset for...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/1026#issuecomment-174012240
First of all travis saw a compilation failure.
```
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR :
[INFO] -------------------------------------------------------------
[ERROR] /home/travis/build/apache/storm/external/storm-kafka/src/jvm/org/apache/storm/kafka/KafkaSpout.java:[103,39] cannot find symbol
symbol: method setLatestEmittedOffset(org.apache.storm.kafka.Partition,long)
location: variable _kafkaOffsetMetric of type org.apache.storm.kafka.KafkaUtils.KafkaOffsetMetric
[ERROR] /home/travis/build/apache/storm/external/storm-kafka/src/jvm/org/apache/storm/kafka/trident/TridentKafkaEmitter.java:[68,27] cannot find symbol
symbol: method setLatestEmittedOffset(org.apache.storm.kafka.Partition,java.lang.Long)
location: variable _kafkaOffsetMetric of type org.apache.storm.kafka.KafkaUtils.KafkaOffsetMetric
```
---
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.
---