----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20471/#review40772 -----------------------------------------------------------
Could you add some comments at the beginning of TopicConfigManager to make it clear that the TopicConfigChange command directly updates the topic config path in ZK and the notification path is just for refreshing the topic config cache in each broker? This wasn't very clear to me after reading the current comment. core/src/main/scala/kafka/server/TopicConfigManager.scala <https://reviews.apache.org/r/20471/#comment73897> The child list returned by ZK doesn't guarantee any ordering. We will need to sort this list so that we don't miss the latest config change. core/src/main/scala/kafka/server/TopicConfigManager.scala <https://reviews.apache.org/r/20471/#comment73896> changeZnode is unused. core/src/main/scala/kafka/server/TopicConfigManager.scala <https://reviews.apache.org/r/20471/#comment73899> In the corner case, it seems that it's still possible by the time that we try to read this ZK path, it's deleted by another broker. We probably need to handle the ZKNodeNotExistException. core/src/main/scala/kafka/server/TopicConfigManager.scala <https://reviews.apache.org/r/20471/#comment73900> It seems that if there are no new config changes, the last few config changes in the notification path will not be deleted. This can be a bit confusing. core/src/main/scala/kafka/server/TopicConfigManager.scala <https://reviews.apache.org/r/20471/#comment73898> This is not needed. In ZK, new sequential nodes always get a higher id whether previous sequential nodes are deleted or not. ZK server maintains enough info to achieve that. core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala <https://reviews.apache.org/r/20471/#comment73901> Missing Apache header. - Jun Rao On April 18, 2014, 12:36 a.m., Jay Kreps wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20471/ > ----------------------------------------------------------- > > (Updated April 18, 2014, 12:36 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1398 > https://issues.apache.org/jira/browse/KAFKA-1398 > > > Repository: kafka > > > Description > ------- > > KAFKA-1398 dynamic config changes are broken. > > > Diffs > ----- > > core/src/main/scala/kafka/server/TopicConfigManager.scala > d41fd33d91406dfa2ce8c1e1b04a078e983ccadd > core/src/test/scala/unit/kafka/server/DynamicConfigChangeTest.scala > PRE-CREATION > > Diff: https://reviews.apache.org/r/20471/diff/ > > > Testing > ------- > > > Thanks, > > Jay Kreps > >
