[GitHub] [pulsar] codelipenghui commented on a diff in pull request #17041: [monitoring][broker][metadata] add metadata store metrics

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

codelipenghui commented on code in PR #17041:
URL: https://github.com/apache/pulsar/pull/17041#discussion_r972893211


##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -58,18 +59,22 @@
 import org.apache.pulsar.metadata.api.extended.MetadataStoreExtended;
 import org.apache.pulsar.metadata.api.extended.SessionEvent;
 import org.apache.pulsar.metadata.cache.impl.MetadataCacheImpl;
+import org.apache.pulsar.metadata.impl.stats.MetadataStoreStats;
 
 @Slf4j
 public abstract class AbstractMetadataStore implements MetadataStoreExtended, Consumer<Notification> {
 
+    private static final AtomicInteger ID = new AtomicInteger();

Review Comment:
   We don't need this one any more?



##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/api/MetadataStoreConfig.java:
##########
@@ -29,6 +29,9 @@
 @Getter
 @ToString
 public class MetadataStoreConfig {
+    public static final String LOCAL_METADATA_STORE = "local-metadata-store";
+    public static final String STATE_METADATA_STORE = "state-metadata-store";

Review Comment:
   Interesting, I don't know we have a zookeeper-based function state store
   
   I think we don't need to have a separate name for this one? because it will use the same zookeeper with the metadata store right?
   
   And I would suggest changing "local-metadata-store" -> "metadata-store", just to keep consistent with the configuration name that we have in the broker.conf.
   



##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java:
##########
@@ -80,9 +85,9 @@ public abstract class AbstractMetadataStore implements MetadataStoreExtended, Co
 
     protected abstract CompletableFuture<Boolean> existsFromStore(String path);
 
-    protected AbstractMetadataStore() {
-        this.executor = Executors
-                .newSingleThreadScheduledExecutor(new DefaultThreadFactory("metadata-store"));
+    protected AbstractMetadataStore(String metadataStoreName) {
+        final String poolName = "metadata-store";
+        this.executor = new ScheduledThreadPoolExecutor(1, new DefaultThreadFactory(poolName));

Review Comment:
   We don't need this change?



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