You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/06/24 17:06:36 UTC
[GitHub] [kafka] vvcephei opened a new pull request, #12342: KAFKA-10888: Revert #12049 f7db603
vvcephei opened a new pull request, #12342:
URL: https://github.com/apache/kafka/pull/12342
Benchmarking indicates that #12049 was responsible for a 10% performance regression in the Producer. We are not sure why, but reverting the commit shows a return to nominal performance, so there is a strong indication that this one commit is indeed the culprit.
Results:
Commit: e3202b9999 (the parent of the problematic commit)
TPut: 118k±1k
Commit: f7db6031b8 (#12049)
TPut: 106k±1k
(every commit that we have tested from trunk after f7db6031b8 is also around 106k)
This PR, reverting f7db6031b8
TPut: 116k±1k
We propose to first revert the commit in question and then re-introduce the feature later, rather than trying to debug and fix the feature in trunk. The longer this commit stays live, the more code will be based on it, and the harder it will be in the future to extract, debug, or fix it.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
vvcephei commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165767443
There is already a new conflict, which seems to support the argument to revert as soon as possible. I'll rebase and update this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165789457
Thanks @vvcephei , I agree it's better to first revert and then re-introduce the feature after we got the root cause of the perf regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] artemlivshits commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
artemlivshits commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165838742
I'm out of office till the end of the week, I can take a look next week. Instead of revert, can we consider setting `partitioner.class=org.apache.kafka.clients.producer.internals.DefaultPartitioner` in producer config? This should restore old behavior. Reverting and re-introducing the change may result in a bunch of merge churn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
vvcephei commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165770584
Also cc @guozhangwang , @ableegoldman , @bbejeck : I had to partially revert some of your changes that had been based on top of the problematic commit. I'm not sure whether you'll want to take a quick look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
junrao commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1166319802
Hmm, just changing partitioner.class is not enough. We also need to change the config doc, revert the deprecation of DefaultPartitioner, and revert https://issues.apache.org/jira/browse/KAFKA-13880
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
vvcephei commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165768336
Pinging @artemlivshits and @junrao for reviews of this revert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
jolshan commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165801074
I'm a little confused by the results here.
> Results:
Commit: https://github.com/apache/kafka/commit/e3202b99999ef4c63aab2e5ab049978704282792 (the parent of the problematic commit)
TPut: 118k±1k
Commit: https://github.com/apache/kafka/commit/f7db6031b84a136ad0e257df722b20faa7c37b8a (https://github.com/apache/kafka/pull/12049)
TPut: 106k±1k
(every commit that we have tested from trunk after https://github.com/apache/kafka/commit/f7db6031b84a136ad0e257df722b20faa7c37b8a is also around 106k)
This PR, reverting https://github.com/apache/kafka/commit/f7db6031b84a136ad0e257df722b20faa7c37b8a
TPut: 116k±1k
it seems like reverting this commit doesn't help that much to improve perf. I'm also wondering what benchmark you are running here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
vvcephei commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165841554
Thanks, all. If we can simply set the default back to the old behavior, then I agree it's nicer than reverting.
Also, of course, I have no intention of hitting "merge" unless we agree it's the right thing to do. I just wanted to raise the priority of this issue before it impacts Kafka users or becomes hopelessly entangled in later commits.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] jolshan commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
jolshan commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165828269
Can we also share more details about the benchmark that was run?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] YiDing-Duke commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
YiDing-Duke commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165811246
@artemlivshits can you review this revert when get a chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
ijuma commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165807769
@vvcephei @guozhangwang Please hold off reverting this until we have had a chance to review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
vvcephei commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1182501741
No need to revert, since this is being fixed in https://github.com/apache/kafka/pull/12365
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei closed pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
vvcephei closed pull request #12342: KAFKA-10888: Revert #12049 f7db603
URL: https://github.com/apache/kafka/pull/12342
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] guozhangwang commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165789978
Made a pass on just my PR regarding the streams partitioner, LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #12342: KAFKA-10888: Revert #12049 f7db603
Posted by GitBox <gi...@apache.org>.
vvcephei commented on PR #12342:
URL: https://github.com/apache/kafka/pull/12342#issuecomment-1165842337
Hey @jolshan , let me share those details with you offline. I would like to publish the benchmarks at some point, but now is not the time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org