You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "hudeqi (via GitHub)" <gi...@apache.org> on 2023/05/11 06:16:17 UTC

[GitHub] [kafka] hudeqi opened a new pull request, #13705: MINOR:Optimize the use of metrics in ReplicaManager and remove checks

hudeqi opened a new pull request, #13705:
URL: https://github.com/apache/kafka/pull/13705

   Inspired by #13623, the code of the metric in ReplicaManager is optimized (to improve the commit of #13471).


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hudeqi commented on pull request #13705: MINOR:Refactor the metric names into constants in ReplicaManager

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1593287087

   > > It seems that the failure of CI check has nothing to do with this pr. @divijvaidya
   > 
   > Hmm...not sure about that. There are 99 tests failing and I am not comfortable saying that they are not because of this PR. Let's wait a day or 2 for CI to stabilize, after that we can merge from trunk again and try the CI for this PR.
   
   Is this CI test often unstable? I've hardly ever seen anything that can be completely checked successfully. 😂


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] divijvaidya merged pull request #13705: MINOR:Refactor the metric names into constants in ReplicaManager

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya merged PR #13705:
URL: https://github.com/apache/kafka/pull/13705


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hudeqi commented on pull request #13705: MINOR:Refactor the metric names into constants in ReplicaManager

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1591356625

   > Looks good to me. One minor thing is to update the PR name (since we use the same for commit messages). As I understand, we are not really optimizing anything here. We are refactoring the metric names into constants. 
   > 
   > Can you please change the PR description to reflect the same? 
   
   ok, I see, updated the pr name. @divijvaidya 


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] divijvaidya commented on pull request #13705: MINOR:Optimize the use of metrics in ReplicaManager and remove checks

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1569926096

   @hudeqi you requested the review today but I think there are some existing comments above that have not yet been addressed


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] divijvaidya commented on a diff in pull request #13705: MINOR:Optimize the use of metrics in ReplicaManager and remove checks

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13705:
URL: https://github.com/apache/kafka/pull/13705#discussion_r1194033848


##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -369,9 +369,9 @@ class ReplicaManagerTest {
       // Use the second instance of metrics group that is constructed. The first instance is constructed by
       // ReplicaManager constructor > BrokerTopicStats > BrokerTopicMetrics.
       val mockMetricsGroup = mockMetricsGroupCtor.constructed.get(1)
-      verify(mockMetricsGroup, times(9)).newGauge(anyString(), any())
-      verify(mockMetricsGroup, times(3)).newMeter(anyString(), anyString(), any(classOf[TimeUnit]))
-      verify(mockMetricsGroup, times(12)).removeMetric(anyString())
+      verify(mockMetricsGroup, times(ReplicaManager.GaugeMetricNames.size)).newGauge(anyString(), any())

Review Comment:
   echo'ing a comment that David provided in https://github.com/apache/kafka/pull/13623#discussion_r1182595446, we can replace the `anyString()` with actual names to verify that the argument passed are correct.



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hudeqi commented on pull request #13705: MINOR:Optimize the use of metrics in ReplicaManager and remove checks

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1591092269

   Hi, @divijvaidya,  it has been updated according to your comment, thank you!


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] divijvaidya commented on a diff in pull request #13705: MINOR:Optimize the use of metrics in ReplicaManager and remove checks

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13705:
URL: https://github.com/apache/kafka/pull/13705#discussion_r1199022749


##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -369,9 +369,12 @@ class ReplicaManagerTest {
       // Use the second instance of metrics group that is constructed. The first instance is constructed by
       // ReplicaManager constructor > BrokerTopicStats > BrokerTopicMetrics.
       val mockMetricsGroup = mockMetricsGroupCtor.constructed.get(1)
-      verify(mockMetricsGroup, times(9)).newGauge(anyString(), any())
-      verify(mockMetricsGroup, times(3)).newMeter(anyString(), anyString(), any(classOf[TimeUnit]))
-      verify(mockMetricsGroup, times(12)).removeMetric(anyString())
+      verify(mockMetricsGroup, times(ReplicaManager.GaugeMetricNames.size)).newGauge(anyString(), any())
+      ReplicaManager.GaugeMetricNames.foreach(metricName => verify(mockMetricsGroup).newGauge(ArgumentMatchers.eq(metricName), any()))
+      verify(mockMetricsGroup, times(ReplicaManager.MeterMetricNames.size)).newMeter(anyString(), anyString(), any(classOf[TimeUnit]))

Review Comment:
   ditto



##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -369,9 +369,12 @@ class ReplicaManagerTest {
       // Use the second instance of metrics group that is constructed. The first instance is constructed by
       // ReplicaManager constructor > BrokerTopicStats > BrokerTopicMetrics.
       val mockMetricsGroup = mockMetricsGroupCtor.constructed.get(1)
-      verify(mockMetricsGroup, times(9)).newGauge(anyString(), any())
-      verify(mockMetricsGroup, times(3)).newMeter(anyString(), anyString(), any(classOf[TimeUnit]))
-      verify(mockMetricsGroup, times(12)).removeMetric(anyString())
+      verify(mockMetricsGroup, times(ReplicaManager.GaugeMetricNames.size)).newGauge(anyString(), any())

Review Comment:
   This line could be removed. The line below is verifying that all entries in `GaugeMetricNames` are having a function invocation.



##########
core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:
##########
@@ -369,9 +369,12 @@ class ReplicaManagerTest {
       // Use the second instance of metrics group that is constructed. The first instance is constructed by
       // ReplicaManager constructor > BrokerTopicStats > BrokerTopicMetrics.
       val mockMetricsGroup = mockMetricsGroupCtor.constructed.get(1)
-      verify(mockMetricsGroup, times(9)).newGauge(anyString(), any())
-      verify(mockMetricsGroup, times(3)).newMeter(anyString(), anyString(), any(classOf[TimeUnit]))
-      verify(mockMetricsGroup, times(12)).removeMetric(anyString())
+      verify(mockMetricsGroup, times(ReplicaManager.GaugeMetricNames.size)).newGauge(anyString(), any())
+      ReplicaManager.GaugeMetricNames.foreach(metricName => verify(mockMetricsGroup).newGauge(ArgumentMatchers.eq(metricName), any()))
+      verify(mockMetricsGroup, times(ReplicaManager.MeterMetricNames.size)).newMeter(anyString(), anyString(), any(classOf[TimeUnit]))
+      ReplicaManager.MeterMetricNames.foreach(metricName => verify(mockMetricsGroup).newMeter(ArgumentMatchers.eq(metricName), anyString(), any(classOf[TimeUnit])))
+      verify(mockMetricsGroup, times(ReplicaManager.MetricNames.size)).removeMetric(anyString())

Review Comment:
   ditto



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hudeqi commented on pull request #13705: MINOR:Refactor the metric names into constants in ReplicaManager

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1593228168

   > I triggered the CI tests again since last run wasn't complete. Will merge this in after they are successful.
   
   It seems that the failure of CI check has nothing to do with this pr. @divijvaidya 


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] divijvaidya commented on pull request #13705: MINOR:Refactor the metric names into constants in ReplicaManager

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1595672796

   Failing tests are unrelated. These are frequently failing tests in trunk.
   ```
   [Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationBaseTest.testReplicateSourceDefault()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/org.apache.kafka.connect.mirror.integration/MirrorConnectorsIntegrationBaseTest/Build___JDK_17_and_Scala_2_13___testReplicateSourceDefault__/)
   [Build / JDK 17 and Scala 2.13 / kafka.api.ConsumerBounceTest.testClose()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.api/ConsumerBounceTest/Build___JDK_17_and_Scala_2_13___testClose__/)
   [Build / JDK 17 and Scala 2.13 / kafka.api.TransactionsTest.testBumpTransactionalEpoch(String).quorum=kraft](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.api/TransactionsTest/Build___JDK_17_and_Scala_2_13___testBumpTransactionalEpoch_String__quorum_kraft/)
   [Build / JDK 17 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_17_and_Scala_2_13____1__Type_ZK__Name_testNewAndChangedTopicsInDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT/)
   [Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest.testReplicateSourceDefault()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/org.apache.kafka.connect.mirror.integration/IdentityReplicationIntegrationTest/Build___JDK_11_and_Scala_2_13___testReplicateSourceDefault__/)
   [Build / JDK 11 and Scala 2.13 / kafka.zk.ZkMigrationIntegrationTest.[1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.zk/ZkMigrationIntegrationTest/Build___JDK_11_and_Scala_2_13____1__Type_ZK__Name_testNewAndChangedTopicsInDualWrite__MetadataVersion_3_4_IV0__Security_PLAINTEXT/)
   [Build / JDK 11 and Scala 2.13 / org.apache.kafka.controller.QuorumControllerTest.testBalancePartitionLeaders()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/org.apache.kafka.controller/QuorumControllerTest/Build___JDK_11_and_Scala_2_13___testBalancePartitionLeaders__/)
   [Build / JDK 8 and Scala 2.12 / kafka.api.ConsumerBounceTest.testClose()](https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13705/11/testReport/junit/kafka.api/ConsumerBounceTest/Build___JDK_8_and_Scala_2_12___testClose__/)
   ```


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] hudeqi commented on pull request #13705: MINOR:Optimize the use of metrics in ReplicaManager and remove checks

Posted by "hudeqi (via GitHub)" <gi...@apache.org>.
hudeqi commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1543404262

   and this, thanks! @divijvaidya @dajac 


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] divijvaidya commented on pull request #13705: MINOR:Refactor the metric names into constants in ReplicaManager

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1591550112

   I triggered the CI tests again since last run wasn't complete. Will merge this in after they are successful. 


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] divijvaidya commented on pull request #13705: MINOR:Refactor the metric names into constants in ReplicaManager

Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13705:
URL: https://github.com/apache/kafka/pull/13705#issuecomment-1593276292

   > It seems that the failure of CI check has nothing to do with this pr. @divijvaidya
   
   Hmm...not sure about that. There are 99 tests failing and I am not comfortable saying that they are not because of this PR. Let's wait a day or 2 for CI to stabilize, after that we can merge from trunk again and try the CI for this 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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org