You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/23 05:06:39 UTC
[GitHub] [pulsar] merlimat opened a new pull request #12150: PIP-45: Only run ZKSessionWatcher when MetadataStoreExtended is required
merlimat opened a new pull request #12150:
URL: https://github.com/apache/pulsar/pull/12150
### Motivation
When we're creating a `MetadataStore` instance that does not require session awareness (eg: when used for configuration store), we can omit the session watcher machinery. That is not getting used and it's adding noise in the logs
--
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
[GitHub] [pulsar] merlimat commented on a change in pull request #12150: PIP-45: Only run ZKSessionWatcher when MetadataStoreExtended is required
Posted by 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
[GitHub] [pulsar] merlimat merged pull request #12150: PIP-45: Only run ZKSessionWatcher when MetadataStoreExtended is required
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #12150:
URL: https://github.com/apache/pulsar/pull/12150
--
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #12150: PIP-45: Only run ZKSessionWatcher when MetadataStoreExtended is required
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12150:
URL: https://github.com/apache/pulsar/pull/12150#discussion_r715115278
##########
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:
Okay
--
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
[GitHub] [pulsar] eolivelli commented on a change in pull request #12150: PIP-45: Only run ZKSessionWatcher when MetadataStoreExtended is required
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #12150:
URL: https://github.com/apache/pulsar/pull/12150#discussion_r714736077
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/common/naming/NamespaceBundleFactory.java
##########
@@ -50,7 +50,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public class NamespaceBundleFactory {
+public class NamespaceBundleFactory {
Review comment:
nit: remove spaces
##########
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:
if enableSessionWatcher is false we can skip adding the watcher at all, can't we ?
--
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