-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[FLINK-16383] Set close = true in monitor/lock scope in KafkaProducer #12151
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.
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
Conversation
Before, the flag was changed outside the lock, which would allow the case that the flag is set to true while someone else is holding the "close lock".
|
Thanks for the patch, Aljoscha. LGTM. |
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit c38ce62 (Thu May 14 12:12:01 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
For debugging FLINK-16383 we need to see who closes a Producer to try and match the "already closed" exceptions.
This should be disabled again after FLINK-16383 has been resolved.
|
@becketqin I added some debug logging. |
StephanEwen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good with one minor comment.
| closed = true; | ||
| synchronized (producerClosingLock) { | ||
| kafkaProducer.close(); | ||
| LOG.debug("Closed internal KafkaProducer {}. Stacktrace: {}", System.identityHashCode(this), Joiner.on("\n").join(Thread.currentThread().getStackTrace())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses eager string concatenation in a lazy parameter, which is typically a blocker.
Now, this is not a performance critical path, but I would still suggest to avoid this, because it sets a bad example for other contributors to immitate (in then more critical parts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's why I left it there but I agree that we should set good examples? So I should put an if (LOG.isDebugEnabled()) {} around it?
|
Thanks for the swift reviews! |
Before, the flag was changed outside the lock, which would allow the
case that the flag is set to true while someone else is holding the
"close lock".