[GitHub] [pulsar] merlimat commented on a change in pull request #12150: PIP-45: Only run ZKSessionWatcher when MetadataStoreExtended is required

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

merlimat commented on a change in pull request #12150:
URL: https://github.com/apache/pulsar/pull/12150#discussion_r714926039



##########
File path: pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/ZKMetadataStore.java
##########
@@ -73,13 +74,15 @@ public ZKMetadataStore(String metadataURL, MetadataStoreConfig metadataStoreConf
                     .allowReadOnlyMode(metadataStoreConfig.isAllowReadOnlyOperations())
                     .sessionTimeoutMs(metadataStoreConfig.getSessionTimeoutMillis())
                     .watchers(Collections.singleton(event -> {

Review comment:
       There's a bit of a chicken and egg problem there that would make the change a bit uglier. The ZKSessionWatcher needs to have the already build ZK client object, while we need to pass the list of watchers to construct the ZK client.
   
   That means that when we build  the ZK client we still don't have the set of watchers (that is also happening in the current implementation, which is why the field is not `final` and we're reading the variable out. 
   
   In practice, I don't think it makes any difference to have the watcher that just ignores the session event instead of no watcher.




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