[GitHub] [kafka] rondagostino commented on pull request #11066: MINOR: Test ReplicaManager Metric Names

2021-07-15 Thread GitBox 


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

2021-07-15 Thread GitBox 


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

2021-07-15 Thread GitBox 


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