KAFKA-4942 Fix commitTimeoutMs being set before the commit actually started #2912
Conversation
See KAFKA-4942
There was a problem hiding this comment.
@56quarters Thanks for working on this! The test looks good, just have one refinement that I think will make it more robust.
| @Override | ||
| public void run() { | ||
| try { | ||
| latch.await(); |
There was a problem hiding this comment.
I think the only issue with this test is that it relies on the fact that we happen to initialize commitStarted to -1, MockTime initializes to the current time (which is a huge time difference to commitStarted), and the fact that this is the first commit. The test as is doesn't trigger if we happen to initialize commitRecord to the current time, though we would want to catch regressions even if we did that.
I see two options. The first is to just add a note about this with an assertion on the starting value of commitStarted. This would at least make it so we would catch the problem if that assumption changed.
The alternative is to simulate that enough time has passed that this issue would be triggered regardless. Since we already are using MockTime in this test, this should be as easy as adding
time.sleep(WorkerConfig.OFFSET_COMMIT_TIMEOUT_MS_DEFAULT);
along with one of the initial iterations to simulate that it took long enough to trigger the problem even if commitStarted is not initialized to -1.
There was a problem hiding this comment.
Added a call to time.sleep between the first and second iterations. Thanks!
There was a problem hiding this comment.
Actually @ewencp, it seems like your concerns are to verify that offset commits are marked as timed out when they do in fact timeout (no matter what commitStarted is initialized to). However, this test is to make sure that commits are not marked as timed out immediately, within the same .iteration() call (with the commit starting on line 162 and then immediately being stopped on line 174). Have I misunderstood your concerns (or how this section of code works)? Thanks!
|
Hi @ewencp Thanks for taking the time to review this. I made the suggested change (adding a call to However, it seems to me, that this change is aimed at making this test more robust if this test were to ensure a timeout actually occurs. However, this test is to ensure that a timeout does not occur where it previously, erroneously did. Thoughts? Thanks again for your help. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
@ewencp I think we should be good to go with this one. Thoughts?
…started This fixes KAFKA-4942 This supersededs #2730 /cc simplesteph gwenshap ewencp Author: Nick Pillitteri <nickp@smartertravelmedia.com> Author: simplesteph <stephane.maarek@gmail.com> Reviewers: simplesteph <stephane.maarek@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>, Ewen Cheslack-Postava <ewen@confluent.io> Closes #2912 from 56quarters/fix-connect-offset-commit (cherry picked from commit d655d80) Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>
|
Excellent, @56quarters! |
|
Thanks! |
This fixes KAFKA-4942
This supersededs #2730
/cc @simplesteph @gwenshap @ewencp