[GitHub] [kafka] showuon commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
showuon commented on pull request #10811: URL: https://github.com/apache/kafka/pull/10811#issuecomment-885307107 Let me handle 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
showuon commented on pull request #10811: URL: https://github.com/apache/kafka/pull/10811#issuecomment-883412685 Make sense. `KAFKA-13108: Improve the test coverage for ConfigCommandTest` is created for it. 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #10811: KAFKA-12598: ConfigCommand should only support communication via ZooKeeper for a reduced set of cases
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
