[GitHub] [pulsar] Jason918 commented on a change in pull request #12930: [Issue 12929] [pulsar-metadata] Fix metadata store deadlock

[GitBox <gi...@apache.org>]

Jason918 commented on a change in pull request #12930:
URL: https://github.com/apache/pulsar/pull/12930#discussion_r759807194



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -1434,7 +1434,7 @@ protected void internalDeleteSubscription(AsyncResponse asyncResponse, String su
             internalDeleteSubscriptionForNonPartitionedTopic(asyncResponse, subName, authoritative);
         } else {
             getPartitionedTopicMetadataAsync(topicName,
-                    authoritative, false).thenAccept(partitionMetadata -> {
+                    authoritative, false).thenAcceptAsync(partitionMetadata -> {

Review comment:
       Thx for the detailed explanation.
   
   
   
   
   
   
   
   > It is OK to block in the call to internalDeleteSubscriptionForNonPartitionedTopic in Line 1434 as that code does not execute in the metadata-store callback thread (it executes in the REST handler).
   
   Agreed on "not execute in the metadata-store callback thread", but it's always not good to execute blocking calls on executor. Maybe it's a little out of the scope of this issue. 
   
   > The scope is fairly large impacting a large swath of the code.
   
   Actually, I think we already have most of them in async mode.  
   
   
   > It also doesn't solve the problem entirely, as there are other blocking calls in the thenAccept lambda. For e.g.,:
   
   Do you mean `waitForAll` ?  It's not blocking.
   
   
   
   
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org