MINOR: Revert "KAFKA-13542: add rebalance reason in Kafka Streams (#11804)" #11873
Conversation
This file contains 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
This reverts commit 2ccc834.
cadonna
approved these changes
Mar 10, 2022
There was a problem hiding this comment.
@wcarlson5 I approve the PR.
However, I -- as probably also you -- do not see the connection to the issue.
|
Restarted builds |
|
@wcarlson5 It would be great if we could get to the bottom on this. I don't really understand how that could impact the performances. |
|
Have you verified that this revert actually fixes the regression? |
|
We should not revert this without providing more detailed information, the PR doesn't include enough information. |
|
Is this method being called in a hot loop? One difference is that there is a string concatenation if the reason is not null. |
|
@dajac @ijuma Hey I expanded the PR description a bit to include everything we know. I also was not sure how this causes a performance impact. It seems innocuous but i has a clear impact on our benchmarks. @lihaosky Is looking to find out more. It looks like a buffer in the network client was overflowing maybe? Commit 2ccc834 (the original) Commit 59ca166 (the revert) Let me know if there is anything else you would like to know |
|
Test failures are unrelated. Going to merge Thanks for digging into this @wcarlson5 and @lihaosky . I hope we can figure out the cause and fix it soon as I was looking forward to having the rebalance reason be available in the JoinGroup :/ |
|
Thanks for the details @wcarlson5 |
ableegoldman
approved these changes
Mar 10, 2022
|
Merged to trunk |
wcarlson5
added a commit
to confluentinc/kafka
that referenced
this pull request
Mar 22, 2022
)" (apache#11873) This reverts commit 2ccc834. This reverts commit 2ccc834. We were seeing serious regressions in our state heavy benchmarks. We saw that our state heavy benchmarks were experiencing a really bad regression. The State heavy benchmarks runs with rolling bounces with 10 nodes. We regularly saw this exception: java.lang.OutOfMemoryError: Java heap space I ran through a git bisect and found this commit. We verified that the commit right before did not have the same issues as this one did. I then reverted the problematic commit and ran the benchmarks again on this commit and did not see any more issues. We are still looking into the root cause, but for now since this isn't a critical improvement so we can remove it temporarily. Reviewers: Bruno Cadonna <cadonna@confluent.io>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>, David Jacot <djacot@confluent.io>, Ismael Juma <ismael@confluent.io>
wcarlson5
added a commit
to confluentinc/kafka
that referenced
this pull request
Mar 28, 2022
)" (apache#11873) This reverts commit 2ccc834. This reverts commit 2ccc834. We were seeing serious regressions in our state heavy benchmarks. We saw that our state heavy benchmarks were experiencing a really bad regression. The State heavy benchmarks runs with rolling bounces with 10 nodes. We regularly saw this exception: java.lang.OutOfMemoryError: Java heap space I ran through a git bisect and found this commit. We verified that the commit right before did not have the same issues as this one did. I then reverted the problematic commit and ran the benchmarks again on this commit and did not see any more issues. We are still looking into the root cause, but for now since this isn't a critical improvement so we can remove it temporarily. Reviewers: Bruno Cadonna <cadonna@confluent.io>, Anna Sophie Blee-Goldman <ableegoldman@apache.org>, David Jacot <djacot@confluent.io>, Ismael Juma <ismael@confluent.io>
3 tasks
dajac
added a commit
that referenced
this pull request
Mar 31, 2022
…upRequest (#11971) KIP-800 introduced a mechanism to pass a reason in the join group request and in the leave group request. A default reason is used unless one is provided by the user. In this case, the custom reason is prefixed by the default one. When we tried to used this in Kafka Streams, we noted a significant degradation of the performances, see #11873. It is not clear wether the prefixing is the root cause of the issue or not. To be on the safe side, I think that we should remove the prefixing. It does not bring much anyway as we are still able to distinguish a custom reason from the default one on the broker side. This patch removes prefixing the user provided reasons. So if a the user provides a reason, the reason is used directly. If the reason is empty or null, the default reason is used. Reviewers: Luke Chen <showuon@gmail.com>, <jeff.kim@confluent.io>, Hao Li <hli@confluent.io>
dajac
added a commit
that referenced
this pull request
Mar 31, 2022
…upRequest (#11971) KIP-800 introduced a mechanism to pass a reason in the join group request and in the leave group request. A default reason is used unless one is provided by the user. In this case, the custom reason is prefixed by the default one. When we tried to used this in Kafka Streams, we noted a significant degradation of the performances, see #11873. It is not clear wether the prefixing is the root cause of the issue or not. To be on the safe side, I think that we should remove the prefixing. It does not bring much anyway as we are still able to distinguish a custom reason from the default one on the broker side. This patch removes prefixing the user provided reasons. So if a the user provides a reason, the reason is used directly. If the reason is empty or null, the default reason is used. Reviewers: Luke Chen <showuon@gmail.com>, <jeff.kim@confluent.io>, Hao Li <hli@confluent.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This reverts commit 2ccc834. We were seeing serious regressions in our state heavy benchmarks. We saw that our state heavy benchmarks were experiencing a really bad regression. The State heavy benchmarks runs with rolling bounces with 10 nodes.
We regularly saw this exception
I ran through a git bisect and found this commit. We verified that the commit right before did not have the same issues as this one did. I then reverted the problematic commit and ran the benchmarks again on this commit and did not see any more issues. We are still looking into the root cause, but for now since this isn't a critical improvement so we can remove it temporarily.
Committer Checklist (excluded from commit message)