Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
showuon merged PR #15483: URL: https://github.com/apache/kafka/pull/15483 -- 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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
chia7712 commented on code in PR #15483: URL: https://github.com/apache/kafka/pull/15483#discussion_r1533141970 ## clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java: ## @@ -40,15 +48,29 @@ public KafkaMetric(Object lock, MetricName metricName, MetricValueProvider va this.time = time; } +/** + * Get the configuration of this metric. + * This is supposed to be used by server only. Review Comment: @mimaison it seems to me those methods are used by server to "write" something (for example, quota), and so users should view the metrics as readonly object. Please correct me If I misunderstand the purpose. -- 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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
mimaison commented on code in PR #15483: URL: https://github.com/apache/kafka/pull/15483#discussion_r1532394640 ## clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java: ## @@ -40,15 +48,29 @@ public KafkaMetric(Object lock, MetricName metricName, MetricValueProvider va this.time = time; } +/** + * Get the configuration of this metric. + * This is supposed to be used by server only. Review Comment: What do you mean by `This is supposed to be used by server only.`? Same for the other method below -- 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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
chia7712 commented on PR #15483: URL: https://github.com/apache/kafka/pull/15483#issuecomment-2009231142 the failed tests are shown below, and they pass on my local ```script ./gradlew cleanTest :tools:test --tests ListConsumerGroupTest.testListConsumerGroupsWithTypesClassicProtocol --tests DescribeConsumerGroupTest.testDescribeStateWithMultiPartitionTopicAndMultipleConsumers :storage:test --tests TransactionsWithTieredStoreTest.testFencingOnAddPartitions --tests TransactionsWithTieredStoreTest.testCommitTransactionTimeout :connect:runtime:test --tests org.apache.kafka.connect.integration.OffsetsApiIntegrationTest.testAlterSinkConnectorOffsets --tests org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSourceConnector --tests org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testPotentialDeadlockWhenProducingToOffsetsTopic --tests org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testSinkConnector --tests org.apache.kafka.connect.integration.ExactlyOnceSourceIntegrationTest.testTasksFailOnInabilityToFence --tests org.apache.kafka.connect.integration.OffsetsApiIntegrationTest.testGetSinkConnectorOf fsetsDifferentKafkaClusterTargeted :metadata:test --tests QuorumControllerTest.testBootstrapZkMigrationRecord :trogdor:test --tests CoordinatorTest.testTaskRequestWithOldStartMsGetsUpdated :server:test --tests ClientMetricsManagerTest.testCacheEviction :core:test --tests DelegationTokenEndToEndAuthorizationWithOwnerTest.testProduceConsumeTopicAutoCreateTopicCreateAcl --tests AuthorizerIntegrationTest.shouldSendSuccessfullyWhenIdempotentAndHasCorrectACL --tests AuthorizerIntegrationTest.testAuthorizeByResourceTypeMultipleAddAndRemove --tests AuthorizerIntegrationTest.testConsumeWithTopicWrite --tests ControllerRegistrationManagerTest.testWrongIncarnationId --tests ReplicaManagerTest.testRemoteFetchExpiresPerSecMetric --tests LogDirFailureTest.testIOExceptionDuringLogRoll --tests LogDirFailureTest.testIOExceptionDuringCheckpoint :clients:test --tests EagerConsumerCoordinatorTest.testOutdatedCoordinatorAssignment ``` -- 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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1530243082
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -78,6 +111,10 @@ public Measurable measurable() {
}
}
+/**
+ * Set the metric config.
Review Comment:
i see, thanks for the explanation.
Just updated it, too.
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
chia7712 commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1530225439
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -78,6 +109,11 @@ public Measurable measurable() {
}
}
+/**
+ * Set the metric config.
+ * This is supposed to be used by server only, DO NOT use this for your
metrics.
Review Comment:
`This is supposed to be used by server only` this description is good enough
to me
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
chia7712 commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1530224467
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -78,6 +111,10 @@ public Measurable measurable() {
}
}
+/**
+ * Set the metric config.
Review Comment:
> However, why are users not allowed to fetch the config?
`MetricConfig` has the methods used by server too, so encouraging user to
access `MetricConfig` could lead potential trouble I feel.
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1528503006
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -78,6 +111,10 @@ public Measurable measurable() {
}
}
+/**
+ * Set the metric config.
Review Comment:
I understand that for the config setter, users should not use it because
it's aimed for server-side caller. However, why are users not allowed to fetch
the config?
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
chia7712 commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1527554674
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -78,6 +111,10 @@ public Measurable measurable() {
}
}
+/**
+ * Set the metric config.
Review Comment:
This method normally is used to update quota and it should be used by
server-side only. Could you add comments to remind `MetricsReporter` users that
they should NOT call this method? ditto for `config()` method
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1527554056
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -40,15 +48,30 @@ public KafkaMetric(Object lock, MetricName metricName,
MetricValueProvider va
this.time = time;
}
+/**
+ * Get the configuration of this metric
+ * @return Return the config of this metric
+ */
public MetricConfig config() {
-return this.config;
+synchronized (lock) {
Review Comment:
this is addressed in [#15550](https://github.com/apache/kafka/pull/15550)
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1527553634
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -40,15 +48,30 @@ public KafkaMetric(Object lock, MetricName metricName,
MetricValueProvider va
this.time = time;
}
+/**
+ * Get the configuration of this metric
+ * @return Return the config of this metric
+ */
public MetricConfig config() {
-return this.config;
+synchronized (lock) {
Review Comment:
sure, let me address this. I think you are right, this is a different issue.
Thanks for pointing this out!
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
chia7712 commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1527537826
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -40,15 +48,30 @@ public KafkaMetric(Object lock, MetricName metricName,
MetricValueProvider va
this.time = time;
}
+/**
+ * Get the configuration of this metric
+ * @return Return the config of this metric
+ */
public MetricConfig config() {
-return this.config;
+synchronized (lock) {
Review Comment:
Could you please file another jira to trace it? This fix is good but it
should be in another PR.
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on PR #15483: URL: https://github.com/apache/kafka/pull/15483#issuecomment-2002480150 tested with ``` ./gradlew cleanTest core:test --tests ConnectionQuotasTest --tests ControllerMutationQuotaTest stream:test --tests StandbyTaskTest --tests StreamTaskTest ``` and it passed -- 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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1527503839
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -40,15 +48,28 @@ public KafkaMetric(Object lock, MetricName metricName,
MetricValueProvider va
this.time = time;
}
+/**
+ * Get the configuration of this metric
+ * @return Return the config of this metric
+ */
public MetricConfig config() {
return this.config;
Review Comment:
I think you are right, there is a lock for setter, we should have a lock for
getter as well.
Thanks for the suggestion, let me add that
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
chia7712 commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1521949713
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -40,15 +48,28 @@ public KafkaMetric(Object lock, MetricName metricName,
MetricValueProvider va
this.time = time;
}
+/**
+ * Get the configuration of this metric
+ * @return Return the config of this metric
+ */
public MetricConfig config() {
return this.config;
Review Comment:
this is unrelate to this PR, but should we add lock to this method? It seems
we assume the config can be read/write concurrently.
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1521282961
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -28,7 +59,14 @@ public final class KafkaMetric implements Metric {
private final MetricValueProvider metricValueProvider;
private MetricConfig config;
-// public for testing
Review Comment:
thanks for the comment!
I see, let me add that back
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
johnnychhsu commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1521280887
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -20,6 +20,37 @@
import org.apache.kafka.common.MetricName;
import org.apache.kafka.common.utils.Time;
+/**
+ * An implementation of {@link Metric} interface.
+ *
+ * A KafkaMetric is a named metric for monitoring purpose. The metric value
can be a {@link Measurable} or a {@link Gauge}.
+ *
+ * metricName The name of the metric
+ * lock A lock used for reading the metric value in case of race
condition
+ * time The POSIX time in milliseconds the metric is being taken
+ * metricValueProvider The metric collecting implementation that
implements {@link MetricValueProvider}
+ * config The metric configuration which is a {@link MetricConfig}
+ *
Review Comment:
thanks for the suggestion! let me remove that
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
mimaison commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1521121701
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -28,7 +59,14 @@ public final class KafkaMetric implements Metric {
private final MetricValueProvider metricValueProvider;
private MetricConfig config;
-// public for testing
Review Comment:
Let's not remove this comment. It's useful to indicate why this constructor
is not private.
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -20,6 +20,37 @@
import org.apache.kafka.common.MetricName;
import org.apache.kafka.common.utils.Time;
+/**
+ * An implementation of {@link Metric} interface.
+ *
+ * A KafkaMetric is a named metric for monitoring purpose. The metric value
can be a {@link Measurable} or a {@link Gauge}.
+ *
+ * metricName The name of the metric
+ * lock A lock used for reading the metric value in case of race
condition
+ * time The POSIX time in milliseconds the metric is being taken
+ * metricValueProvider The metric collecting implementation that
implements {@link MetricValueProvider}
+ * config The metric configuration which is a {@link MetricConfig}
+ *
Review Comment:
Yeah since we don't expect users to create KafkaMetric objects I don't think
we need this.
--
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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
showuon commented on PR #15483: URL: https://github.com/apache/kafka/pull/15483#issuecomment-1990842126 @mimaison , please take a look when available. Thanks. -- 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
Re: [PR] KAFKA-16318 : add javafoc for kafka metric [kafka]
showuon commented on code in PR #15483:
URL: https://github.com/apache/kafka/pull/15483#discussion_r1520919788
##
clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java:
##
@@ -20,6 +20,37 @@
import org.apache.kafka.common.MetricName;
import org.apache.kafka.common.utils.Time;
+/**
+ * An implementation of {@link Metric} interface.
+ *
+ * A KafkaMetric is a named metric for monitoring purpose. The metric value
can be a {@link Measurable} or a {@link Gauge}.
+ *
+ * metricName The name of the metric
+ * lock A lock used for reading the metric value in case of race
condition
+ * time The POSIX time in milliseconds the metric is being taken
+ * metricValueProvider The metric collecting implementation that
implements {@link MetricValueProvider}
+ * config The metric configuration which is a {@link MetricConfig}
+ *
Review Comment:
This is already described in constructor, so I think we don't need it. WDYT?
--
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
