Skip to content

Conversation

@C0urante
Copy link
Contributor

@C0urante C0urante commented Jun 8, 2022

Jira

Also added some <p> tags to help organize the rendered Javadocs.

Committer Checklist (excluded from commit message)

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

Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Thanks @C0urante! @hachikuji does this look OK to you?

Comment on lines 762 to 765
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* If the transaction is committed successfully and this method returns without throwing an exception, it is guaranteed
* that all {@link Callback callbacks} for records in the transaction will have been invoked and completed, either
* successfully or by raising an exception. In the event that a callback fails (i.e., raises an exception), the producer
* will still proceed with the transaction commit.
* If the transaction is committed successfully and this method returns without throwing an exception, it is guaranteed
* that all {@link Callback callbacks} for records in the transaction will have been invoked. Exceptions thrown by
* callbacks are ignored; the producer proceeds to commit the transaction in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, "invoked" means that the method has been called but not necessarily that it has completed (i.e., returned or thrown an exception), and given the multi-threaded nature of the producer, I'd like it if we could make that distinction so that there's no longer any room for doubt in users' minds on that front.

Fine with the new language in the last sentence though 👍

Copy link
Contributor

@hachikuji hachikuji Jun 8, 2022

Choose a reason for hiding this comment

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

I don't think we allow a commit to proceed if any of the sends fail. The user has to abort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does a failing callback qualify as a failing send? Otherwise I'm not sure what part of this implies that a transaction will proceed if a send fails, and the previous paragraph explicitly calls that out already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think my confusion came from the phrase "either successfully or by raising an exception." I thought this was referring to the callback result (i.e. what we are passing to Callback.onCompletion). Perhaps we could just leave that part out since the next sentence is clarifying anyway?

If the transaction is committed successfully and this method returns without throwing an exception, it is guaranteed that all {@link Callback callbacks} for records in the transaction will have been invoked and completed. Note that exceptions thrown by callbacks are ignored; the producer proceeds to commit the transaction in any case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, done.

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@hachikuji hachikuji merged commit 7692643 into apache:trunk Jun 10, 2022
@C0urante C0urante deleted the patch-1 branch June 10, 2022 17:15
rajinisivaram added a commit to confluentinc/kafka that referenced this pull request Jun 12, 2022
…-2022

* apache/trunk: (52 commits)
  KAFKA-13967: Document guarantees for producer callbacks on transaction commit (apache#12264)
  [KAFKA-13848] Clients remain connected after SASL re-authentication f… (apache#12179)
  KAFKA-10000: Zombie fencing logic (apache#11779)
  KAFKA-13947: Use %d formatting for integers rather than %s (apache#12267)
  KAFKA-13929: Replace legacy File.createNewFile() with NIO.2 Files.createFile() (apache#12197)
  KAFKA-13780: Generate OpenAPI file for Connect REST API (apache#12067)
  KAFKA-13917: Avoid calling lookupCoordinator() in tight loop (apache#12180)
  KAFKA-10199: Implement removing active and standby tasks from the state updater (apache#12270)
  MINOR: Update Scala to 2.13.8 in gradle.properties (apache#12273)
  MINOR: add java 8/scala 2.12 deprecation info in doc (apache#12261)
  ...

 Conflicts:
	gradle.properties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants