[GitHub] [kafka] showuon commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases

[GitBox <gi...@apache.org>]

showuon commented on pull request #10811:
URL: https://github.com/apache/kafka/pull/10811#issuecomment-882996715


   @ijuma , thanks for the update. It looks better now! Also, thank you and @rondagostino for your patiently review!
   
   For your question:
   > it looks to me that we don't have test coverage in ConfigCommandTest for the case where we try to update broker configs via zk when the brokers are up, is that right?
   
   I actually added a test for it:
   `shouldNotAllowAddBrokerQuotaConfigWhileBrokerUpUsingZookeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L905)
   
   And this test is to test update multiple broker configs using zookeeper (when no brokers up)
   `testDynamicBrokerConfigUpdateUsingZooKeeper` [here](https://github.com/apache/kafka/blob/da252c633d88ecec6f68200081ad94a3081e5f35/core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala#L1245)
   
   Do you think we should add more tests for it?


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