asafm commented on code in PR #17041:
URL: https://github.com/apache/pulsar/pull/17041#discussion_r973681673
##########
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:
>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`.
I agree with that. Today the user knows this from configuration:
`metadataStoreUrl` and `configurationMetadataStoreUrl` so when they see a label
name, they need to be able to match it with what they configured, no?
##########
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:
Shouldn't the poolName be as metadataStore name?
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java:
##########
@@ -1522,6 +1524,81 @@ public void testSplitTopicAndPartitionLabel() throws
Exception {
consumer2.close();
}
+
+ @Test
+ public void testMetadataStoreStats() throws Exception {
Review Comment:
You resolved without answering
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/stats/MetadataStoreStats.java:
##########
@@ -0,0 +1,110 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.metadata.impl.stats;
+
+import io.prometheus.client.Counter;
+import io.prometheus.client.Histogram;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+public final class MetadataStoreStats implements AutoCloseable {
+ private static final double[] BUCKETS = new double[]{1, 3, 5, 10, 20, 50,
100, 200, 500, 1000, 2000, 5000};
+ private static final String OPS_TYPE_LABEL_NAME = "type";
+ private static final String METADATA_STORE_LABEL_NAME = "name";
+ private static final String STATUS = "status";
+
+ private static final String OPS_TYPE_GET = "get";
+ private static final String OPS_TYPE_DEL = "del";
+ private static final String OPS_TYPE_PUT = "put";
+ private static final String STATUS_SUCCESS = "success";
+ private static final String STATUS_FAIL = "fail";
+
+ protected static final String PREFIX = "pulsar_metadata_store_";
+
+ private static final Histogram OPS_LATENCY = Histogram
+ .build(PREFIX + "ops_latency", "-")
+ .unit("ms")
+ .buckets(BUCKETS)
+ .labelNames(METADATA_STORE_LABEL_NAME, OPS_TYPE_LABEL_NAME, STATUS)
+ .register();
+ private static final Counter PUT_BYTES = Counter
+ .build(PREFIX + "put", "-")
+ .unit("bytes")
+ .labelNames(METADATA_STORE_LABEL_NAME)
+ .register();
+
+ private final Histogram.Child getOpsSucceedChild;
+ private final Histogram.Child delOpsSucceedChild;
+ private final Histogram.Child putOpsSucceedChild;
+ private final Histogram.Child getOpsFailedChild;
+ private final Histogram.Child delOpsFailedChild;
+ private final Histogram.Child putOpsFailedChild;
+ private final Counter.Child putBytesChild;
+ private final String metadataStoreName;
+ private final AtomicBoolean closed = new AtomicBoolean(false);
+
+ public MetadataStoreStats(String metadataStoreName) {
+ this.metadataStoreName = metadataStoreName;
+
+ this.getOpsSucceedChild = OPS_LATENCY.labels(metadataStoreName,
OPS_TYPE_GET, STATUS_SUCCESS);
+ this.delOpsSucceedChild = OPS_LATENCY.labels(metadataStoreName,
OPS_TYPE_DEL, STATUS_SUCCESS);
+ this.putOpsSucceedChild = OPS_LATENCY.labels(metadataStoreName,
OPS_TYPE_PUT, STATUS_SUCCESS);
+ this.getOpsFailedChild = OPS_LATENCY.labels(metadataStoreName,
OPS_TYPE_GET, STATUS_FAIL);
+ this.delOpsFailedChild = OPS_LATENCY.labels(metadataStoreName,
OPS_TYPE_DEL, STATUS_FAIL);
+ this.putOpsFailedChild = OPS_LATENCY.labels(metadataStoreName,
OPS_TYPE_PUT, STATUS_FAIL);
+ this.putBytesChild = PUT_BYTES.labels(metadataStoreName);
+ }
+
+ public void recordGetOpsLatency(long millis) {
Review Comment:
I know we had this discussion before, but I'm just adding a small note here
before this PR gets merged: don't you prefer for consistency's sake to have
`recordGetOpsSucceeded` which matches with `recordGetOpsFailed`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
