[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled
divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1282860735
##
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##
@@ -122,8 +123,6 @@ public class RemoteLogManager implements Closeable {
private static final Logger LOGGER =
LoggerFactory.getLogger(RemoteLogManager.class);
private static final String REMOTE_LOG_READER_THREAD_NAME_PREFIX =
"remote-log-reader";
Review Comment:
shouldn't this go to RemoteStorageMetrics as well?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled
divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1282856425
##
core/src/test/scala/integration/kafka/api/MetricsTest.scala:
##
@@ -54,6 +57,12 @@ class MetricsTest extends IntegrationTestHarness with
SaslSetup {
@BeforeEach
override def setUp(testInfo: TestInfo): Unit = {
+if (testInfo.getDisplayName.endsWith("true")) {
Review Comment:
Wouldn't this enable RemoteStorage for all tests which use `@Parameterized`
(and not just for testMetrics)? An alternative way is to add the full test name
here OR check for string "systemRemoteStorageEnabled: true".
##
core/src/main/java/kafka/log/remote/RemoteLogManager.java:
##
@@ -122,8 +123,6 @@ public class RemoteLogManager implements Closeable {
private static final Logger LOGGER =
LoggerFactory.getLogger(RemoteLogManager.class);
private static final String REMOTE_LOG_READER_THREAD_NAME_PREFIX =
"remote-log-reader";
Review Comment:
shouldn't this go to RemoteStorageMetrics as well?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled
divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1281718684
##
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageMetrics.java:
##
@@ -0,0 +1,93 @@
+/*
+ * 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.kafka.server.log.remote.storage;
+
+import com.yammer.metrics.core.MetricName;
+import org.apache.kafka.server.metrics.KafkaYammerMetrics;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * This class contains the metrics related to tiered storage feature, which is
to have a centralized
+ * place to store them, so that we can verify all of them easily.
+ *
+ * @see kafka.api.MetricsTest
+ */
+public class RemoteStorageMetrics {
+public static final String REMOTE_LOG_READER_METRICS_NAME_PREFIX =
"RemoteLogReader";
+public static final String REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT =
"RemoteLogManagerTasksAvgIdlePercent";
+public static final String TASK_QUEUE_SIZE = "TaskQueueSize";
+public static final String AVG_IDLE_PERCENT = "AvgIdlePercent";
+public static final String REMOTE_BYTES_OUT_PER_SEC =
"RemoteBytesOutPerSec";
+public static final String REMOTE_BYTES_IN_PER_SEC = "RemoteBytesInPerSec";
+public static final String REMOTE_READ_REQUESTS_PER_SEC =
"RemoteReadRequestsPerSec";
+public static final String REMOTE_WRITE_REQUESTS_PER_SEC =
"RemoteWriteRequestsPerSec";
+public static final String FAILED_REMOTE_READ_REQUESTS_PER_SEC =
"RemoteReadErrorsPerSec";
+public static final String FAILED_REMOTE_WRITE_REQUESTS_PER_SEC =
"RemoteWriteErrorsPerSec";
+public static final String REMOTE_LOG_READER_TASK_QUEUE_SIZE =
REMOTE_LOG_READER_METRICS_NAME_PREFIX + TASK_QUEUE_SIZE;
+public static final String REMOTE_LOG_READER_AVG_IDLE_PERCENT =
REMOTE_LOG_READER_METRICS_NAME_PREFIX + AVG_IDLE_PERCENT;
+
+public final static MetricName REMOTE_BYTES_OUT_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics", REMOTE_BYTES_OUT_PER_SEC);
+public final static MetricName REMOTE_BYTES_IN_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics", REMOTE_BYTES_IN_PER_SEC);
+public final static MetricName REMOTE_READ_REQUESTS_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics",
REMOTE_READ_REQUESTS_PER_SEC);
+public final static MetricName REMOTE_WRITE_REQUESTS_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics",
REMOTE_WRITE_REQUESTS_PER_SEC);
+public final static MetricName FAILED_REMOTE_READ_REQUESTS_PER_SEC_METRIC
= getMetricName(
+"kafka.server", "BrokerTopicMetrics",
FAILED_REMOTE_READ_REQUESTS_PER_SEC);
+public final static MetricName FAILED_REMOTE_WRITE_REQUESTS_PER_SEC_METRIC
= getMetricName(
+"kafka.server", "BrokerTopicMetrics",
FAILED_REMOTE_WRITE_REQUESTS_PER_SEC);
+public final static MetricName
REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT_METRIC = getMetricName(
+"kafka.log.remote", "RemoteLogManager",
REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT);
+public final static MetricName REMOTE_LOG_READER_TASK_QUEUE_SIZE_METRIC =
getMetricName(
+"org.apache.kafka.storage.internals.log",
"RemoteStorageThreadPool", REMOTE_LOG_READER_TASK_QUEUE_SIZE);
+public final static MetricName REMOTE_LOG_READER_AVG_IDLE_PERCENT_METRIC =
getMetricName(
+"org.apache.kafka.storage.internals.log",
"RemoteStorageThreadPool", REMOTE_LOG_READER_AVG_IDLE_PERCENT);
+
+public static Set allMetrics() {
Review Comment:
the metrics you added in the last commit. e.g.
`FAILED_REMOTE_WRITE_REQUESTS_PER_SEC` is missing and hence, our integration
tests is not validating that this metric is not emitted when Tiered Storage is
turned off.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled
divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1281642972
##
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageMetrics.java:
##
@@ -0,0 +1,93 @@
+/*
+ * 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.kafka.server.log.remote.storage;
+
+import com.yammer.metrics.core.MetricName;
+import org.apache.kafka.server.metrics.KafkaYammerMetrics;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * This class contains the metrics related to tiered storage feature, which is
to have a centralized
+ * place to store them, so that we can verify all of them easily.
+ *
+ * @see kafka.api.MetricsTest
+ */
+public class RemoteStorageMetrics {
+public static final String REMOTE_LOG_READER_METRICS_NAME_PREFIX =
"RemoteLogReader";
+public static final String REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT =
"RemoteLogManagerTasksAvgIdlePercent";
+public static final String TASK_QUEUE_SIZE = "TaskQueueSize";
+public static final String AVG_IDLE_PERCENT = "AvgIdlePercent";
+public static final String REMOTE_BYTES_OUT_PER_SEC =
"RemoteBytesOutPerSec";
+public static final String REMOTE_BYTES_IN_PER_SEC = "RemoteBytesInPerSec";
+public static final String REMOTE_READ_REQUESTS_PER_SEC =
"RemoteReadRequestsPerSec";
+public static final String REMOTE_WRITE_REQUESTS_PER_SEC =
"RemoteWriteRequestsPerSec";
+public static final String FAILED_REMOTE_READ_REQUESTS_PER_SEC =
"RemoteReadErrorsPerSec";
+public static final String FAILED_REMOTE_WRITE_REQUESTS_PER_SEC =
"RemoteWriteErrorsPerSec";
+public static final String REMOTE_LOG_READER_TASK_QUEUE_SIZE =
REMOTE_LOG_READER_METRICS_NAME_PREFIX + TASK_QUEUE_SIZE;
+public static final String REMOTE_LOG_READER_AVG_IDLE_PERCENT =
REMOTE_LOG_READER_METRICS_NAME_PREFIX + AVG_IDLE_PERCENT;
+
+public final static MetricName REMOTE_BYTES_OUT_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics", REMOTE_BYTES_OUT_PER_SEC);
+public final static MetricName REMOTE_BYTES_IN_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics", REMOTE_BYTES_IN_PER_SEC);
+public final static MetricName REMOTE_READ_REQUESTS_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics",
REMOTE_READ_REQUESTS_PER_SEC);
+public final static MetricName REMOTE_WRITE_REQUESTS_PER_SEC_METRIC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics",
REMOTE_WRITE_REQUESTS_PER_SEC);
+public final static MetricName FAILED_REMOTE_READ_REQUESTS_PER_SEC_METRIC
= getMetricName(
+"kafka.server", "BrokerTopicMetrics",
FAILED_REMOTE_READ_REQUESTS_PER_SEC);
+public final static MetricName FAILED_REMOTE_WRITE_REQUESTS_PER_SEC_METRIC
= getMetricName(
+"kafka.server", "BrokerTopicMetrics",
FAILED_REMOTE_WRITE_REQUESTS_PER_SEC);
+public final static MetricName
REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT_METRIC = getMetricName(
+"kafka.log.remote", "RemoteLogManager",
REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT);
+public final static MetricName REMOTE_LOG_READER_TASK_QUEUE_SIZE_METRIC =
getMetricName(
+"org.apache.kafka.storage.internals.log",
"RemoteStorageThreadPool", REMOTE_LOG_READER_TASK_QUEUE_SIZE);
+public final static MetricName REMOTE_LOG_READER_AVG_IDLE_PERCENT_METRIC =
getMetricName(
+"org.apache.kafka.storage.internals.log",
"RemoteStorageThreadPool", REMOTE_LOG_READER_AVG_IDLE_PERCENT);
+
+public static Set allMetrics() {
Review Comment:
we are missing the newly added metrics here.
##
storage/api/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteStorageMetrics.java:
##
@@ -0,0 +1,93 @@
+/*
+ * 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
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled
divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1280583213
##
core/src/main/java/kafka/log/remote/RemoteStorageMetrics.java:
##
@@ -0,0 +1,66 @@
+/*
+ * 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 kafka.log.remote;
+
+import com.yammer.metrics.core.MetricName;
+import kafka.server.BrokerTopicStats;
+import org.apache.kafka.server.metrics.KafkaYammerMetrics;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import static
org.apache.kafka.storage.internals.log.RemoteStorageThreadPool.AVG_IDLE_PERCENT;
+import static
org.apache.kafka.storage.internals.log.RemoteStorageThreadPool.TASK_QUEUE_SIZE;
+
+public class RemoteStorageMetrics {
+final static MetricName REMOTE_BYTES_OUT_PER_SEC = getMetricName(
+"kafka.server", "BrokerTopicMetrics",
BrokerTopicStats.RemoteBytesOutPerSec());
+final static MetricName REMOTE_BYTES_IN_PER_SEC = getMetricName(
+"kafka.server", "BrokerTopicMetrics",
BrokerTopicStats.RemoteBytesInPerSec());
+final static MetricName REMOTE_READ_REQUESTS_PER_SEC = getMetricName(
+"kafka.server", "BrokerTopicMetrics",
BrokerTopicStats.RemoteReadRequestsPerSec());
+final static MetricName REMOTE_WRITE_REQUESTS_PER_SEC = getMetricName(
+"kafka.server", "BrokerTopicMetrics",
BrokerTopicStats.RemoteWriteRequestsPerSec());
+final static MetricName FAILED_REMOTE_READ_REQUESTS_PER_SEC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics",
BrokerTopicStats.FailedRemoteReadRequestsPerSec());
+final static MetricName FAILED_REMOTE_WRITE_REQUESTS_PER_SEC =
getMetricName(
+"kafka.server", "BrokerTopicMetrics",
BrokerTopicStats.FailedRemoteWriteRequestsPerSec());
+final static MetricName REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT =
getMetricName(
+"kafka.log.remote", "RemoteLogManager",
RemoteLogManager.REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT);
+final static MetricName REMOTE_LOG_READER_TASK_QUEUE_SIZE = getMetricName(
+"org.apache.kafka.storage.internals.log", "RemoteStorageThreadPool",
RemoteLogManager.REMOTE_LOG_READER_METRICS_NAME_PREFIX + TASK_QUEUE_SIZE);
Review Comment:
Apologies that I wasn't clear in my previous comments. I was hoping that
RemoteStorageMetrics act as the source of truth for all RemoteStorage related
metrics, i.e. instead of `BrokerTopicStats.FailedRemoteWriteRequestsPerSec` we
have `RemoteStorageMetrics.FailedRemoteWriteRequestsPerSec` and this constant
is used in BrokerTopicStats. This is also consistent with how we are defining
metrics in newer code such as QuorumControllerMetrics.
As an example for REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT:
we have a constant
```
public final String REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT_METRIC_NAME =
"RemoteLogManagerTasksAvgIdlePercent";
final static MetricName REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT =
getMetricName(
"kafka.log.remote", "RemoteLogManager",
RemoteStorageMetrics.REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT);
```
At RemoteLogManager, we use the interface of MetricsRegistry which takes
MetricNames as an argument:
```
public Gauge newGauge(MetricName metricName, Gauge metric)
```
and use it as:
```
metricsGroup.newGauge(RemoteStorageMetrics.REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT,
new Gauge() {
@Override
public Double value() {
return rlmScheduledThreadPool.getIdlePercent();
}
});
```
The advantages of this approach is that:
1. we don't have metrics constants defined at two places, e.g. MetricsNames
in RemoteStorageMetrics and String metric names in respective files.
2. when someone adds a new metric RemoteStorage based metric, they will
notice that all metrics are at one place and will remember to add their new
metric in this file.
3. we use a consistent approach across the code base such as in
`QuorumControllerMetrics` and `ControllerMetadataMetrics`
4. Later the scope of responsibility of this class could be expanded to
close all RemoteStorage metrics at one place simultaneously.
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled
divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1280395279
##
core/src/main/java/kafka/log/remote/RemoteStorageMetrics.java:
##
@@ -0,0 +1,53 @@
+/*
+ * 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 kafka.log.remote;
+
+import com.yammer.metrics.core.MetricName;
+import org.apache.kafka.server.metrics.KafkaYammerMetrics;
+
+import java.util.HashSet;
+import java.util.Set;
+
+public class RemoteStorageMetrics {
Review Comment:
I was hoping that we could add all metric related to Remote Storage here
(and validate that they aren't initialized). Metrics which are missing are the
ones such as REMOTE_LOG_MANAGER_TASKS_AVG_IDLE_PERCENT and others which are
only applicable if Remote Storage is turned on at system level.
##
core/src/main/java/kafka/log/remote/RemoteStorageMetrics.java:
##
@@ -0,0 +1,53 @@
+/*
+ * 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 kafka.log.remote;
+
+import com.yammer.metrics.core.MetricName;
+import org.apache.kafka.server.metrics.KafkaYammerMetrics;
+
+import java.util.HashSet;
+import java.util.Set;
+
+public class RemoteStorageMetrics {
+final static MetricName REMOTE_BYTES_OUT_PER_SEC = getMetricName(
+"BrokerTopicMetrics", "RemoteBytesOutPerSec");
+final static MetricName REMOTE_BYTES_IN_PER_SEC = getMetricName(
+"BrokerTopicMetrics", "RemoteBytesInPerSec");
+final static MetricName REMOTE_READ_REQUESTS_PER_SEC = getMetricName(
+"BrokerTopicMetrics", "RemoteReadRequestsPerSec");
+final static MetricName REMOTE_WRITE_REQUESTS_PER_SEC = getMetricName(
+"BrokerTopicMetrics", "RemoteWriteRequestsPerSec");
Review Comment:
should we re-use constants here `BrokerTopicStats.RemoteWriteRequestsPerSec`
(if they are available)
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #14133: KAFKA-15189: only init remote topic metrics when enabled
divijvaidya commented on code in PR #14133:
URL: https://github.com/apache/kafka/pull/14133#discussion_r1279101884
##
core/src/main/scala/kafka/server/KafkaRequestHandler.scala:
##
@@ -227,7 +227,7 @@ class KafkaRequestHandlerPool(val brokerId: Int,
}
}
-class BrokerTopicMetrics(name: Option[String]) {
+class BrokerTopicMetrics(name: Option[String], systemRemoteStorageEnabled:
Boolean) {
Review Comment:
In it's current form, this is not very extensible for future cases because
if we want to optionally add metrics for a new feature, we will have to add new
booleans as arguments to determine the usage of that feature.
Perhaps we can either pass the entire config object and let this
`BrokerTopicMetrics` class make a decision based on config on what metrics it
want to enable.
##
core/src/test/scala/kafka/server/KafkaRequestHandlerTest.scala:
##
@@ -77,4 +79,28 @@ class KafkaRequestHandlerTest {
assertEquals(Some(startTime + 200),
request.callbackRequestDequeueTimeNanos)
assertEquals(Some(startTime + 300),
request.callbackRequestCompleteTimeNanos)
}
+
+
+ @ParameterizedTest
+ @ValueSource(booleans = Array(true, false))
+ def testTopicStats(systemRemoteStorageEnabled: Boolean): Unit = {
Review Comment:
I would feel more confident if we have a test, that spins up the servers
without Remote Storage enabled and verifies that no Remote Storage related
metrics are registered. To do this, we will have to collect all Remote Storage
metrics in a class called "RemoteStorageMetrics" similar to
QuorumControllerMetrics and use that class as source of truth for all metrics
names related to RemoteStorage. In the test, we can validate that none of the
metrics related to RemoteStorage is registered in the default registry.
##
core/src/main/scala/kafka/server/KafkaRequestHandler.scala:
##
@@ -439,12 +441,14 @@ class BrokerTopicStats extends Logging {
topicMetrics.closeMetric(BrokerTopicStats.ProduceMessageConversionsPerSec)
topicMetrics.closeMetric(BrokerTopicStats.ReplicationBytesOutPerSec)
topicMetrics.closeMetric(BrokerTopicStats.ReassignmentBytesOutPerSec)
- topicMetrics.closeMetric(BrokerTopicStats.RemoteBytesOutPerSec)
- topicMetrics.closeMetric(BrokerTopicStats.RemoteBytesInPerSec)
- topicMetrics.closeMetric(BrokerTopicStats.RemoteReadRequestsPerSec)
- topicMetrics.closeMetric(BrokerTopicStats.RemoteWriteRequestsPerSec)
- topicMetrics.closeMetric(BrokerTopicStats.FailedRemoteReadRequestsPerSec)
-
topicMetrics.closeMetric(BrokerTopicStats.FailedRemoteWriteRequestsPerSec)
+ if (systemRemoteStorageEnabled) {
Review Comment:
closeMetric is a no-op if metric is missing, hence we don't need to do a
check here.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
