KAFKA-7285: Create new producer on each rebalance if EOS enabled#5501
Merged
mjsax merged 6 commits intoapache:trunkfrom Aug 16, 2018
Merged
KAFKA-7285: Create new producer on each rebalance if EOS enabled#5501mjsax merged 6 commits intoapache:trunkfrom
mjsax merged 6 commits intoapache:trunkfrom
Conversation
Member
Author
|
Call for review @guozhangwang @bbejeck @vvcephei @hachikuji |
Member
Author
|
Triggered system tests: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1908/ |
Contributor
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
Overall looks fine to me.
I'm a bit concerned about our current inter-twined logic on the close(clean, isZombie) calls. But that is sort of a tech debt and out of this scope.
| public void resume() { | ||
| // nothing to do; new transaction will be started only after topology is initialized | ||
| log.debug("Resuming"); | ||
| if (eosEnabled) { |
Contributor
There was a problem hiding this comment.
nit: above comment can be removed.
Member
Author
|
@guozhangwang Can you create a JIRA about the |
Contributor
|
The system tests failed: http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2018-08-13--001.1534210196--mjsax--kafka-7285-producer-fence-fix--285a344/report.html looking at: Worker got: Picking another random test: Looks like the same failure (worker6): |
bbejeck
approved these changes
Aug 14, 2018
| if (!isZombie && transactionInFlight) { | ||
| producer.abortTransaction(); | ||
| } | ||
| transactionInFlight = false; |
Member
There was a problem hiding this comment.
nit: do we want to consider setting producer to null here as well if eosEnabled? I realize this branch of the code should only get exercised when closing, but just in case we make changes I don't think it will hurt.
Member
Author
|
Updated this. Re-triggered system tests: |
vvcephei
reviewed
Aug 14, 2018
| } | ||
| try { | ||
| if (!isZombie) { | ||
| recordCollector.close(); |
Contributor
There was a problem hiding this comment.
Should we avoid calling recordCollector.close() unless eosEnabled?
It seems like this block used to be guarded by if(eosEnabled), but it's not anymore.
- fixed close logic - added more tests
Member
Author
|
Updated again. Added more test and discovered some more issues with close/suspend and fixed it. |
Member
Author
|
Previous system test passed. Re-triggered again: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1912/ |
Contributor
|
Jenkins failures are relevant? |
Member
Author
|
Seems like... will have a look. |
Member
Author
|
Updated. |
| if (eosEnabled && !isZombie) { | ||
| if (transactionInFlight) { | ||
| try { | ||
| producer.abortTransaction(); |
Contributor
There was a problem hiding this comment.
If suspend fails, we will always call task.close(..) again inside AssignedTasks, in which we will abort transactions and close record collectors. So do we have to do it here?
Member
Author
There was a problem hiding this comment.
That's a good point. If we do the cleanup twice, we might call producer.abortTransaction() and producer.close() again, after a successful producer.close() and thus log unnecessary error message (the exceptions itself would get swallowed).
However, we need to make sure that we don't fail a second time with closeTopology() when task.close(...) calls task.suspend(false,...) a second time.
Will update the PR.
Member
Author
|
Previous system tests passed. Updated and retriggered: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/1913/ |
Member
|
failure unrelated retest this please |
Member
|
@mjsax no futher comments, updated code looks good to me. |
Member
Author
|
Retest this please |
Contributor
|
LGTM. Please feel free to merge after tests pass. |
Member
Author
|
Again Retest this please |
Member
Author
Retest this please |
Member
Author
|
Java8: |
mjsax
added a commit
that referenced
this pull request
Aug 16, 2018
Reviewers: Guozhang Wang <guozhang@confluent.io>, John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
Member
Author
|
Merged to |
mjsax
added a commit
that referenced
this pull request
Aug 16, 2018
Reviewers: Guozhang Wang <guozhang@confluent.io>, John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
guozhangwang
pushed a commit
that referenced
this pull request
Sep 5, 2018
Back porting #5501 broke some tests. Reviewers: John Roesler <vvcephei@users.noreply.github.com>, Guozhang Wang <wangguoz@gmail.com>
pengxiaolong
pushed a commit
to pengxiaolong/kafka
that referenced
this pull request
Jun 14, 2019
…che#5501) Reviewers: Guozhang Wang <guozhang@confluent.io>, John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Committer Checklist (excluded from commit message)