You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by srdo <gi...@git.apache.org> on 2017/07/31 18:46:38 UTC
[GitHub] storm pull request #2249: WIP: STORM-2648/STORM-2357: Add storm-kafka-client...
GitHub user srdo opened a pull request:
https://github.com/apache/storm/pull/2249
WIP: STORM-2648/STORM-2357: Add storm-kafka-client support for at-most-onc…
…e processing and a toggle for whether messages should be emitted with a message id when not using at-least-once
See https://issues.apache.org/jira/browse/STORM-2357 and https://issues.apache.org/jira/browse/STORM-2648.
I'd like to get some opinions on whether this approach is a good idea, or whether I've overlooked a better option, before finishing this up with some tests. I don't love that we'll end up with 3 different committing behaviors.
In 2357 it was noted that the spout doesn't currently support true at-most-once, because using Kafka's auto commit option leaves the possibility that the spout receives a tuple, emits it to the topology, crashes and recovers, and then receives and emits the same tuple. The linked issue suggests solving this by committing polled offsets before emitting them to the topology, which is an option added here.
2648 notes that there is currently no way to make Storm track messages when using auto commit with this spout. This prevents Storm UI from showing the complete latency for the spout, and I would assume also prevents max spout pending from having an effect. I've added a toggle to KafkaSpoutConfig to force the spout to emit messages with message ids, even when using auto commit or the at-most-once option. The spout does nothing on ack or fail when not doing at-least-once.
I'd like to keep the spout config simple for the user, so I've added a processing guarantee setting corresponding to the standard at-least-once code path, the path that uses auto commit, and the path that commits offsets before emitting any tuples.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srdo/storm STORM-2648
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2249.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 #2249
----
commit 4fc4b71f9720f506be20740f780dfef93f2dd036
Author: Stig Rohde Døssing <sr...@apache.org>
Date: 2017-07-31T18:26:55Z
STORM-2648/STORM-2357: Add storm-kafka-client support for at-most-once processing and a toggle for whether messages should be emitted with a message id when not using at-least-once
----
---
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 #2249: WIP: STORM-2648/STORM-2357: Add storm-kafka-client suppor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2249
+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 issue #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2249
@hmcl Are you still reviewing this? It's fine if you don't have time to look at this, but please say so. I'd like to not keep holding this up.
---
[GitHub] storm issue #2249: WIP: STORM-2648/STORM-2357: Add storm-kafka-client suppor...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2249
@srdo reviewing it
---
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 #2249: WIP: STORM-2648/STORM-2357: Add storm-kafka-client suppor...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2249
@srdo can you please assign JIRA's to you and mark them as in progress as you work on them and/or submit a pull request. Thanks.
---
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 #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2249
Thanks :)
---
[GitHub] storm issue #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2249
@hmcl I don't mean to try to rush you, but please let me know if you're still reviewing. If not I'll probably merge in the next few days.
---
[GitHub] storm issue #2249: WIP: STORM-2648/STORM-2357: Add storm-kafka-client suppor...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2249
Conceptually the changes look good to me. I have not dug into it in great detail yet, but I do like the direction of the change.
I would also like to see the documentation and examples updated to reflect the new change.
---
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 #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2249
Merged via https://github.com/apache/storm/commit/48f6969027e7b02a5b9220577189d3911aa2226d
@srdo
Sorry I forgot to add auto close message while squashing commits. Could you close this?
And please craft the patch for 1.x branch since it doesn't looks like a clean cherry-pick.
Thanks in advance!
---
[GitHub] storm issue #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2249
@srdo apologies for the delay. I will finish today.
---
[GitHub] storm pull request #2249: STORM-2648/STORM-2357: Add storm-kafka-client supp...
Posted by srdo <gi...@git.apache.org>.
Github user srdo closed the pull request at:
https://github.com/apache/storm/pull/2249
---
[GitHub] storm issue #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2249
Thanks for reviews. Opened the 1.x version here https://github.com/apache/storm/pull/2353.
---
[GitHub] storm issue #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2249
Still +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 issue #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2249
This PR has been waiting for about 2 months, and once it gets +1 and no -1, it can be merged.
I'm +1 and will just merge.
@hmcl Please vote -1 later and rollback the merge if you have concern about the patch and would want to vote -1.
---
[GitHub] storm issue #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2249
@hmcl Are you reviewing this, or are you satisfied with it?
---
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 #2249: STORM-2648/STORM-2357: Add storm-kafka-client support for...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2249
Added some tests and updated the docs.
---
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 #2249: WIP: STORM-2648/STORM-2357: Add storm-kafka-client suppor...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2249
Yes, I forgot.
---
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.
---