Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STORM-2787: storm-kafka-client KafkaSpout method onPartitionsRevoked(...) should set initialized flag independently of processing guarantees #2387

Merged
merged 2 commits into from
Oct 29, 2017

Conversation

hmcl
Copy link
Contributor

@hmcl hmcl commented Oct 25, 2017

The commit for STORM-2787 is on top of STORM-2781 to make it easier to merge later. Please review only STORM-2787. Once the code reviews for STORM-2781 have been agreed upon I will incorporate them in this patch as well.

@srdo
Copy link
Contributor

srdo commented Oct 25, 2017

+1 for the change. It might be good to add a test that demonstrates the fix as well if possible.

@HeartSaVioR
Copy link
Contributor

Looks like 1.1.x branch also needs the patch, and that branch shouldn't be on top of STORM-2781.
https://github.com/apache/storm/blob/1.1.x-branch/external/storm-kafka-client/src/main/java/org/apache/storm/kafka/spout/KafkaSpout.java

@hmcl Could you craft a new PR against 1.1.x-branch if my understanding is right? Thanks in advance.

 - Define processing guarantees as AT_LEAST_ONCE, AT_MOST_ONCE, NONE
 - Refactor method name from setForceEnableTupleTracking to setTupleTrackingEnforced
 - Throw IllegalStateException instead of IllegalArgumentException if spout attempts to emit an already committed message
 - Update documentation to reflect these changes
…...) should set initialized flag independently of processing guarantees
@hmcl hmcl force-pushed the Apache_master_STORM-2787_KSInitFlag branch from 12473ed to 0b547d3 Compare October 27, 2017 23:04
@hmcl
Copy link
Contributor Author

hmcl commented Oct 27, 2017

@srdo @HeartSaVioR I have incorporated the code review changes of the depending patch. It should be good to merge. Thanks.

@asfgit asfgit merged commit 0b547d3 into apache:master Oct 29, 2017
@HeartSaVioR
Copy link
Contributor

@hmcl
Thanks, I merged into master.
Like STORM-2781 (#2380) we need another PR on 1.x branch. Please raise pull requests against 1.x branch. Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants