chia7712 commented on code in PR #16323:
URL: https://github.com/apache/kafka/pull/16323#discussion_r1641587527


##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -1084,6 +1084,11 @@ class KafkaServer(
     }
   }
 
+  override def isShutdown(): Boolean = {
+    BrokerState.fromValue(brokerState.value()) == BrokerState.SHUTTING_DOWN ||

Review Comment:
   Could you please use scala match pattern? for example:
   ```scala
       BrokerState.fromValue(brokerState.value()) match {
         case BrokerState.SHUTTING_DOWN | BrokerState.NOT_RUNNING => true
         case _ => false
       }
   ```



##########
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##########
@@ -232,4 +233,16 @@ public void 
testNotSupportedNewGroupProtocols(ClusterInstance clusterInstance) {
         
Assertions.assertTrue(clusterInstance.supportedGroupProtocols().contains(CLASSIC));
         Assertions.assertEquals(1, 
clusterInstance.supportedGroupProtocols().size());
     }
+
+    @ClusterTest(brokers = 4)
+    public void testClusterAliveBrokers(ClusterInstance clusterInstance) 
throws Exception {
+        clusterInstance.waitForReadyBrokers();
+        Map<Integer, KafkaBroker> startBrokers = clusterInstance.brokers();
+
+        // Remove broker id 0
+        clusterInstance.shutdownBroker(0);
+        startBrokers.remove(0);

Review Comment:
   We should verify all types and make sure the `brokers` return "all" brokers. 
For example:
   ```java
       @ClusterTest(types = {Type.ZK, Type.CO_KRAFT, Type.KRAFT}, brokers = 4)
       public void testClusterAliveBrokers(ClusterInstance clusterInstance) 
throws Exception {
           clusterInstance.waitForReadyBrokers();
           Map<Integer, KafkaBroker> startBrokers = clusterInstance.brokers();
   
           // Remove broker id 0
           clusterInstance.shutdownBroker(0);
           
Assertions.assertFalse(clusterInstance.aliveBrokers().containsKey(0));
           Assertions.assertTrue(clusterInstance.brokers().containsKey(0));
   
           // add broker id 0 back
           clusterInstance.startBroker(0);
           Assertions.assertTrue(clusterInstance.aliveBrokers().containsKey(0));
           Assertions.assertTrue(clusterInstance.brokers().containsKey(0));
       }
   ```



-- 
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

Reply via email to