[GitHub] [kafka] rondagostino commented on pull request #11066: MINOR: Test ReplicaManager Metric Names
rondagostino commented on pull request #11066: URL: https://github.com/apache/kafka/pull/11066#issuecomment-881063696 Maybe rename `ReplicaManagerMetricNamesTest` to `ClusterMetricNamesTest` so that it isn't specific to `ReplicaManager`? We could define it like this: ``` @ClusterTest def testMetrics(): Unit = { checkReplicaManagerMetrics() // checkWhateverOtherMetricsWeDecideToAddHereLater() } private def checkReplicaManagerMetrics() { // same implementation as we have now } ``` -- 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] rondagostino commented on pull request #11066: MINOR: Test ReplicaManager Metric Names
rondagostino commented on pull request #11066: URL: https://github.com/apache/kafka/pull/11066#issuecomment-881062140 > Can we do the integration test approach, but have it be part of RaftClusterTest.scala? I think we would only be testing for the KRaft case if we did that, and I think we should be testing both KRaft and ZK-based clusters. -- 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] rondagostino commented on pull request #11066: MINOR: Test ReplicaManager Metric Names
rondagostino commented on pull request #11066: URL: https://github.com/apache/kafka/pull/11066#issuecomment-881031042 I've implemented the test in 2 separate ways, and we should choose which one to keep. 1. A standard unit test, in `ReplicaManagerTest` 2. A parameterized test that runs for both KRaft and ZooKeeper metadata quorums. The advantage of (1) is that it is fast -- just a few seconds at most. The disadvantage is that this test alone would not have caught the 2.8 bug where the metrics moved for the KRaft case -- we would have had to explicitly writte a similar test within `RaftReplicaManager`. The advantage of (2) is that it would have caught the bug automatically. The disadvantage is that it takes much longer to run both cases -- between 15 and 20 seconds on my local laptop. -- 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