bharanic-dev commented on a change in pull request #12930:
URL: https://github.com/apache/pulsar/pull/12930#discussion_r759750975
##########
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:
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).
In order to make `internalDeleteSubscriptionForNonPartitionedTopic` async
non-blocking and have it return a completableFuture, the code changes are
extensive. One has to make sure all the code in
`internalDeleteSubscriptionForNonPartitionedTopic` non-blocking. That is
`validateTopicOwnership`, `validateTopicOperation`, `sub.delete().get()` and
all the methods that they in turn invoke. For e.g.:
https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java#L605
The scope is fairly large impacting a large swath of the code.
It also doesn't solve the problem entirely, as there are other blocking
calls in the `thenAccept` lambda. For e.g.,:
https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L1455
In the future, if some one adds a blocking call in the `thenAccept` lambda
the problem will get re-introduced, which is very easily missed in a code
review.
For the above reasons, changing `thenAccept` to `thenAcceptAsync` seems to
be the best option for now.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
