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
[Broker] Fix Future.join()
causing deadlock.
#14469
Conversation
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
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.
@mattisonchao - thanks for opening the PR. I left some feedback.
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
/pulsarbot rerun-failure-checks |
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
/pulsarbot rerun-failure-checks |
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.
Is there any test cases covering this?
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Show resolved
Hide resolved
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.
Lgtm
Can you please find existing tests that cover this change?
I am not sure we have tests that cover all error cases
@Jason918 @eolivelli |
PTAL :) |
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java
Show resolved
Hide resolved
@michaeljmarshall could you please take a final look ? |
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.
@mattisonchao - PTAL. I will approve and merge this once we agree on my comment, thanks!
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
…ersistent/PersistentTopic.java Co-authored-by: Michael Marshall <mikemarsh17@gmail.com>
Just in case anyone is looking for more context, one of my concerns with the initial implementation had to do with unsafe publication of mutable state, which can behave in unexpected ways. I explain this more in this comment #14469 (comment) |
Master issue apache#14438 ### Motivation Invoking the ``join()`` method in the async method will cause some deadlock. ### Modifications - Refactor ``PersistentTopic#tryToDeletePartitionedMetadata`` to pure async.
Master issue #14438
Motivation
Invoking the
join()
method in the async method will cause some deadlock.Modifications
PersistentTopic#tryToDeletePartitionedMetadata
to pure async.Verifying this change
Documentation
no-need-doc