[GitHub] [pulsar] eolivelli commented on a diff in pull request #17638: [fix][metadata] Handle session events in a separate thread

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

eolivelli commented on code in PR #17638:
URL: https://github.com/apache/pulsar/pull/17638#discussion_r970482337


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -409,14 +409,19 @@ public void registerSessionListener(Consumer<SessionEvent> listener) {
 
     protected void receivedSessionEvent(SessionEvent event) {
         isConnected = event.isConnected();
-
-        sessionListeners.forEach(l -> {
-            try {
-                l.accept(event);
-            } catch (Throwable t) {
-                log.warn("Error in processing session event", t);
-            }
-        });
+        try {
+            executor.execute(() -> {

Review Comment:
   One concern that I have with this patch is that usually in ZooKeeper client applications you leverage the feature that all the events are dispatched in strict order.
   Here we are moving the execution of the listeners to another thread, and it actually works as the executor is still single threaded.
   But on the other hand.. aren't we moving the problem to the `metadata-store` ?
   
   maybe the problem is to be fixed in `AutoRecoveryMain.... onSessionExpired()` that is performing a blocking operation (depending on a sync ZooKeeper calll) on the main zookeeper thread. 



-- 
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