Skip to content

MINOR: Logs warning message when user invoke producer#flush within callback#18112

Merged
AndrewJSchofield merged 1 commit intoapache:trunkfrom
frankvicky:add-log-for-producer-network-thread
Dec 10, 2024
Merged

MINOR: Logs warning message when user invoke producer#flush within callback#18112
AndrewJSchofield merged 1 commit intoapache:trunkfrom
frankvicky:add-log-for-producer-network-thread

Conversation

@frankvicky
Copy link
Copy Markdown
Contributor

As title.
Please refer to the discussion of #17946 for further details

Committer Checklist (excluded from commit message)

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

Copy link
Copy Markdown
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@frankvicky thanks for this patch

Comment thread docs/upgrade.html Outdated
</li>
<li>The deprecated <code>sendOffsetsToTransaction(Map&lt;TopicPartition, OffsetAndMetadata&gt;, String)</code> method has been removed from the Producer API.
</li>
<li>The <code>flush</code> method now detects potential deadlocks and logs a warning when used inside a callback.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is unnecessary as it is just a log.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally, I tend the other way on this. If someone is now getting errors logged, this explains why. No big deal either way, I suppose. Just my 2 cents.

@frankvicky frankvicky force-pushed the add-log-for-producer-network-thread branch from ece977f to 4a69ffa Compare December 10, 2024 06:08
Copy link
Copy Markdown
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Just one nit to fix.

* </p>
* <p>
* <b>Important:</b> This method should not be used within the callback provided to
* {@link #send(ProducerRecord, Callback)}.Invoking <code>flush()</code> in this context will cause a deadlock.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: Space before "Invoking" please.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out 😺

@frankvicky frankvicky force-pushed the add-log-for-producer-network-thread branch from 4a69ffa to 3723114 Compare December 10, 2024 09:24
@AndrewJSchofield AndrewJSchofield merged commit f57fd2d into apache:trunk Dec 10, 2024
peterxcli pushed a commit to peterxcli/kafka that referenced this pull request Dec 18, 2024
…llback (apache#18112)

Reviewers: Andrew Schofield <aschofield@confluent.io>
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
…llback (apache#18112)

Reviewers: Andrew Schofield <aschofield@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants