----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34554/#review92917 -----------------------------------------------------------
Ship it! Thanks for the patch. Looks good. +1 after addressing a few minor comments below. Also, could you rebase? core/src/main/scala/kafka/admin/ConfigCommand.scala (lines 134 - 135) <https://reviews.apache.org/r/34554/#comment147186> Topics and Clients should be lower case. Better to reference the constants. core/src/main/scala/kafka/server/TopicConfigManager.scala (lines 35 - 36) <https://reviews.apache.org/r/34554/#comment147183> I think it's better to have the type be "topic" and "client", without s. core/src/main/scala/kafka/utils/ZkUtils.scala (line 769) <https://reviews.apache.org/r/34554/#comment147185> Perhaps it's better to name this getAllEntitiesWithConfig()? - Jun Rao On July 17, 2015, 6:20 p.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34554/ > ----------------------------------------------------------- > > (Updated July 17, 2015, 6:20 p.m.) > > > Review request for kafka, Joel Koshy and Jun Rao. > > > 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 > 6. Addressed all of Jun's comments. > > > Diffs > ----- > > bin/kafka-configs.sh PRE-CREATION > core/src/main/scala/kafka/admin/AdminUtils.scala > 2b4e028f8a60fcae40c42cfabcc357e70e7ff9a6 > core/src/main/scala/kafka/admin/ConfigCommand.scala PRE-CREATION > core/src/main/scala/kafka/admin/TopicCommand.scala > a90aa8787ff21b963765a547980154363c1c93c6 > core/src/main/scala/kafka/cluster/Partition.scala > 2649090b6cbf8d442649f19fd7113a30d62bca91 > core/src/main/scala/kafka/controller/KafkaController.scala > b4fc755641b9bbe8a6bf9c221a9ffaec0b94d6e8 > core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala > bb6b5c8764522e7947bb08998256ce1deb717c84 > 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 > 18917bc4464b9403b16d85d20c3fd4c24893d1d3 > core/src/main/scala/kafka/server/ReplicaFetcherThread.scala > c89d00b5976ffa34cafdae261239934b1b917bfe > core/src/main/scala/kafka/server/TopicConfigManager.scala > 01b1b0a8efe6ab3ddc7bf9f1f535b01be4e2e6be > core/src/main/scala/kafka/utils/ZkUtils.scala > 166814c2959a429e20f400d1c0e523090ce37d91 > core/src/test/scala/unit/kafka/admin/AdminTest.scala > 252ac813c8df1780c2dc5fa9e698fb43bb6d5cf8 > core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala PRE-CREATION > core/src/test/scala/unit/kafka/admin/TopicCommandTest.scala > dcd69881445c29765f66a7d21d2d18437f4df428 > core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala > 8a871cfaf6a534acd1def06a5ac95b5c985b024c > > 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 > >
