Conversation
|
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. |
|
Pinging @artemlivshits and @junrao for reviews of this revert. |
|
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. |
vvcephei
force-pushed
the
john-revert-12049
branch
from
8a3934b to
c17a6f1
Compare
|
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. |
|
Made a pass on just my PR regarding the streams partitioner, LGTM. |
…ache#12049)" This reverts commit f7db603. The main PR for this is apache#12342
|
@vvcephei @guozhangwang Please hold off reverting this until we have had a chance to review. |
|
@artemlivshits can you review this revert when get a chance? |
|
Can we also share more details about the benchmark that was run? |
|
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 |
|
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. |
|
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. |
|
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 |
|
No need to revert, since this is being fixed in #12365 |
7 participants
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: e3202b9 (the parent of the problematic commit)
TPut: 118k±1k
Commit: f7db603 (#12049)
TPut: 106k±1k
(every commit that we have tested from trunk after f7db603 is also around 106k)
This PR, reverting f7db603
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)