----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34554/#review90622 -----------------------------------------------------------
Thanks for the patch. A few more comments below. 1. The patch doesn't apply. Could you rebase? 2. Also, we need the logic to read all existing client configs. Is that in a separate jira? core/src/main/scala/kafka/admin/ConfigCommand.scala (line 49) <https://reviews.apache.org/r/34554/#comment143735> What is the 1 at the end? core/src/main/scala/kafka/admin/ConfigCommand.scala (lines 60 - 106) <https://reviews.apache.org/r/34554/#comment143736> It seems that all those methods can be private. core/src/main/scala/kafka/admin/TopicCommand.scala (line 243) <https://reviews.apache.org/r/34554/#comment143740> The description needs to be changed to reflect what can now be altered. core/src/main/scala/kafka/admin/TopicCommand.scala (line 252) <https://reviews.apache.org/r/34554/#comment143739> Since topic config can't be altered here, we need to change the description accordingly. core/src/main/scala/kafka/admin/TopicCommand.scala (line 258) <https://reviews.apache.org/r/34554/#comment143741> Since we can't alter the config, it seems that we don't need this option at all. core/src/main/scala/kafka/server/ConfigHandler.scala (line 28) <https://reviews.apache.org/r/34554/#comment143742> case k: (TopicAndPartition, Log) => k._1.topic can just be case (topicAndPartition, log) => topicAndPartition.topic core/src/main/scala/kafka/server/ConfigHandler.scala (line 31) <https://reviews.apache.org/r/34554/#comment143743> Not needed? core/src/main/scala/kafka/server/KafkaServer.scala (line 31) <https://reviews.apache.org/r/34554/#comment143750> Do we need JavaConversions? If this is needed, it would be better to import it in the context where the conversion is actually needed. core/src/main/scala/kafka/server/TopicConfigManager.scala (line 133) <https://reviews.apache.org/r/34554/#comment143745> entityType => entity_type. Also, it would be useful to include the original json. core/src/main/scala/kafka/server/TopicConfigManager.scala (line 138) <https://reviews.apache.org/r/34554/#comment143746> Value => entity_name core/src/main/scala/kafka/server/TopicConfigManager.scala (lines 140 - 143) <https://reviews.apache.org/r/34554/#comment143747> Since we have verified the entity type before, we don't need to check the handler exists or not here. core/src/main/scala/kafka/server/TopicConfigManager.scala (line 145) <https://reviews.apache.org/r/34554/#comment143748> It's probably useful to include the orginal json string. Also, could we make the message string in all IllegalArgumentException consistent? For example, they should all reference config change notification. core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala (lines 88 - 96) <https://reviews.apache.org/r/34554/#comment143751> Are we really mocking zkclient calls here? - Jun Rao On July 2, 2015, 1:39 a.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34554/ > ----------------------------------------------------------- > > (Updated July 2, 2015, 1:39 a.m.) > > > Review request for kafka and Joel Koshy. > > > Bugs: KAFKA-2205 > https://issues.apache.org/jira/browse/KAFKA-2205 > > > Repository: kafka > > > Description > ------- > > KAFKA-2205. Summary of changes: > > 1. Generalized TopicConfigManager to DynamicConfigManager. It is now able to > handle multiple types of entities. > 2. Changed format of the notification znode as described in KIP-21 > 3. Replaced TopicConfigManager with DynamicConfigManager. > 4. Added new testcases. Existing testcases all pass > 5. Added ConfigCommand to handle all config changes. Eventually this will > make calls to the broker once the new API's are built for now it speaks to ZK > directly > > > Diffs > ----- > > core/src/main/scala/kafka/admin/AdminUtils.scala > f06edf41c732a7b794e496d0048b0ce6f897e72b > core/src/main/scala/kafka/admin/ConfigCommand.scala PRE-CREATION > core/src/main/scala/kafka/admin/TopicCommand.scala > 8e6f18633b25bf1beee3f813b28ef7aa7d779d7b > core/src/main/scala/kafka/cluster/Partition.scala > 730a232482fdf77be5704cdf5941cfab3828db88 > core/src/main/scala/kafka/controller/KafkaController.scala > 69bba243a9a511cc5292b43da0cc48e421a428b0 > core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala > 3b15ab4eef22c6f50a7483e99a6af40fb55aca9f > core/src/main/scala/kafka/controller/TopicDeletionManager.scala > 64ecb499f24bc801d48f86e1612d927cc08e006d > core/src/main/scala/kafka/server/ConfigHandler.scala PRE-CREATION > core/src/main/scala/kafka/server/KafkaServer.scala > ea6d165d8e5c3146d2c65e8ad1a513308334bf6f > core/src/main/scala/kafka/server/ReplicaFetcherThread.scala > b31b432a226ba79546dd22ef1d2acbb439c2e9a3 > core/src/main/scala/kafka/server/TopicConfigManager.scala > b675a7e45ea4f4179f8b15fe221fd988aff13aa0 > core/src/main/scala/kafka/utils/ZkUtils.scala > 2618dd39b925b979ad6e4c0abd5c6eaafb3db5d5 > core/src/test/scala/unit/kafka/admin/AdminTest.scala > efb2f8e79b3faef78722774b951fea828cd50374 > core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala > c7136f20972614ac47aa57ab13e3c94ef775a4b7 > core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala > 7877f6ca1845c2edbf96d4a9783a07a552db8f07 > > Diff: https://reviews.apache.org/r/34554/diff/ > > > Testing > ------- > > 1. Added new testcases for new code. > 2. Verified that both topic and client configs can be changed dynamically by > starting a local cluster > > > Thanks, > > Aditya Auradkar > >
