Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-10337: await async commits in commitSync even if no offsets given #13678

Merged
merged 1 commit into from Jun 7, 2023
Merged

KAFKA-10337: await async commits in commitSync even if no offsets given #13678

merged 1 commit into from Jun 7, 2023

Conversation

erikvanoosten
Copy link
Contributor

@erikvanoosten erikvanoosten commented May 6, 2023 •
edited

The contract for commitSync() guarantees that the callbacks for all prior async commits will be invoked before it (successfully?) returns. Prior to this change the contract could be violated if an empty offsets map were passed in to commitSync().

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@erikvanoosten
Copy link
Contributor Author

This PR is a copy of #9111 so all credits go to @thomaslee.

@erikvanoosten
Copy link
Contributor Author

The failing tests are not related to this PR.

@erikvanoosten
Copy link
Contributor Author

@philipnee Did you get a chance to look at this PR already?

@erikvanoosten erikvanoosten mentioned this pull request May 10, 2023
Closed
7 tasks
@philipnee
Copy link
Collaborator

Hey @erikvanoosten, Thanks for the PR! Could you add checks for inflightCommits count gets set to 0 in a few of the callback testing function like testAsyncCommitCallbacksInvokedPriorToSyncCommitCompletion ?

@erikvanoosten
Copy link
Contributor Author

Hey @erikvanoosten, Thanks for the PR! Could you add checks for inflightCommits count gets set to 0 in a few of the callback testing function like testAsyncCommitCallbacksInvokedPriorToSyncCommitCompletion ?

Sure, I'll try. I am new in this code base and these changes are not from me, but I'll try nonetheless.

@philipnee
Copy link
Collaborator

@showuon - do you have a cycle to take a look at this issue?

@dajac
Copy link
Contributor

dajac commented May 11, 2023

Another small weirdness is that when commitAsync is called with an empty map, its callback is executed before the ones which are still in-flight. This beaks the contract of the method: Corresponding commit callbacks are also invoked in the same order..

@erikvanoosten
Copy link
Contributor Author

Hey @erikvanoosten, Thanks for the PR! Could you add checks for inflightCommits count gets set to 0 in a few of the callback testing function like testAsyncCommitCallbacksInvokedPriorToSyncCommitCompletion ?

I looked for all invocations of commitAsync and added additional asserts in 1fa48f53f0e6f5a2a9821075fa053e01cba6b0b2.

@philipnee
Copy link
Collaborator

philipnee commented May 17, 2023 •
edited

@erikvanoosten - Thanks for following up on this PR, I think we are really close here. Also apologize about the misleading comment. I left a few comments above.

@erikvanoosten erikvanoosten force-pushed the complete-commits branch 5 times, most recently from ef9ce53 to 1c033b6 Compare May 18, 2023 07:23
@erikvanoosten
Copy link
Contributor Author

@philipnee Shall we merge?

The failing tests are not related to this PR.
I can confirm that the last changes are still performing as expected for my use-case (tested locally).

@philipnee
Copy link
Collaborator

@dajac - Would you have some time to review and help Erik to merge this? I've done a pass and I think it's okay.

@dajac
Copy link
Contributor

dajac commented May 22, 2023

@philipnee I will try to review it this week. Thanks!

@erikvanoosten
Copy link
Contributor Author

@philipnee I will try to review it this week. Thanks!

Hi @dajac, did you already get the chance to look at this PR?

@dajac
Copy link
Contributor

dajac commented May 30, 2023

@erikvanoosten I am sorry but I haven't had the time to get to it yet. Will do!

dajac
dajac reviewed Jun 2, 2023
View reviewed changes
Copy link
Contributor

@dajac dajac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikvanoosten I finally had time to make a pass over it. Overall, it looks good to me. I left one minor comment. Could you take a look?

clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java Outdated Show resolved Hide resolved
philipnee
philipnee approved these changes Jun 2, 2023
View reviewed changes
Copy link
Collaborator

@philipnee philipnee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think it looks good.

@philipnee
Copy link
Collaborator

The failures seem unrelated

Build / JDK 8 and Scala 2.12 / testDescribeReportOverriddenConfigs(String).quorum=kraft – kafka.admin.TopicCommandIntegrationTest
10s
Build / JDK 8 and Scala 2.12 / testDelayedConfigurationOperations() – org.apache.kafka.controller.QuorumControllerTest
<1s
Build / JDK 11 and Scala 2.13 / testSyncTopicConfigs() – org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest
1m 36s
Build / JDK 11 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
12s
Build / JDK 11 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
13s
Fixed 68

@erikvanoosten
Copy link
Contributor Author

erikvanoosten commented Jun 2, 2023 •
edited

🥳 What is the process now? Who will press the merge button?

@philipnee
Copy link
Collaborator

I'm leaving it for David to press that button!

@erikvanoosten
Copy link
Contributor Author

Okay! @dajac if you're fine with 2bfedf0, this can be merged! 🥳

@erikvanoosten
Copy link
Contributor Author

Commits were squashed. No further changes.

@thomaslee @erikvanoosten @philipnee
KAFKA-10337: await async commits in commitSync even if no offsets given
e4ba315 
The contract for commitSync() guarantees that the callbacks for all prior async commits will be invoked before it (successfully?) returns.
Prior to this change the contract could be violated if an empty offsets map were passed in to commitSync().

Co-authored-by: Erik van Oosten <e.vanoosten@grons.nl>
Co-authored-by: Philip Nee <philipnee@gmail.com>
dajac
dajac approved these changes Jun 7, 2023
View reviewed changes
Copy link
Contributor

@dajac dajac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the patch @erikvanoosten! I will merge when the build completes.

@dajac dajac merged commit 59d30a0 into apache:trunk Jun 7, 2023
1 check failed
@dajac
Copy link
Contributor

dajac commented Jun 7, 2023

@erikvanoosten It seems that you don't have an account for jira so I can't assign the ticket to you. You should request one.

@erikvanoosten
Copy link
Contributor Author

@erikvanoosten It seems that you don't have an account for jira so I can't assign the ticket to you. You should request one.

I do have an account. My Jira userid is erikvanoosten.

@erikvanoosten erikvanoosten deleted the complete-commits branch June 7, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirktrue kirktrue kirktrue left review comments

@dajac dajac dajac approved these changes

@philipnee philipnee philipnee approved these changes

Assignees

@erikvanoosten erikvanoosten

Labels
consumer
Projects
None yet
Milestone
No milestone
5 participants
@erikvanoosten @philipnee @dajac @kirktrue @thomaslee