----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34554/#review92008 -----------------------------------------------------------
Thanks for the latest patch. Have a few more comments below. core/src/main/scala/kafka/admin/ConfigCommand.scala (line 34) <https://reviews.apache.org/r/34554/#comment145852> Could we add a ConfigCommand shell script in bin/? core/src/main/scala/kafka/admin/ConfigCommand.scala (line 36) <https://reviews.apache.org/r/34554/#comment145861> In topic command, if we don't provide the topic name, it will describe all topics. Could we add the same capability here? If entity name is not provided, we will just list all entities whose config has been overwritten. core/src/main/scala/kafka/admin/ConfigCommand.scala (lines 126 - 128) <https://reviews.apache.org/r/34554/#comment145851> Currently, the output looks like the following. Could we put cleanup.policy on the next line? bin/kafka-run-class.sh kafka.admin.ConfigCommand Add/Remove entity (topic/client) configs Option Description ------ ----------- --added-config Key Value pairs configs to add 'k1=v1, k2=v2'. The following is a list of valid configurations: For entity_type 'Topics': cleanup.policy compression.type delete.retention.ms file.delete.delay.ms flush.messages core/src/main/scala/kafka/admin/ConfigCommand.scala (line 159) <https://reviews.apache.org/r/34554/#comment145859> entityType -> entity_type core/src/main/scala/kafka/server/ConfigHandler.scala (lines 64 - 65) <https://reviews.apache.org/r/34554/#comment145853> Could we just use Pool? core/src/main/scala/kafka/server/TopicConfigManager.scala (lines 35 - 36) <https://reviews.apache.org/r/34554/#comment145860> The command line description says the entity types are topic/client, without s. We can reference the constants in the description as well. - Jun Rao On July 14, 2015, 5:37 p.m., Aditya Auradkar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34554/ > ----------------------------------------------------------- > > (Updated July 14, 2015, 5:37 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 > ----- > > 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 > 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 > >
