> On Feb. 4, 2014, 11:21 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/server/ReplicaFetcherThread.scala, line 86 > > <https://reviews.apache.org/r/17537/diff/3/?file=456708#file456708line86> > > > > Reading from zookeeper is unnecessary here, since the broker has a > > mechanism to load per topic configs on the fly. Just accessing it through > > the config object should suffice
Here you could just do if(brokerConfig.uncleanLeaderElectionEnable) instead of if (!AdminUtils.canElectUncleanLeaderForTopic(replicaMgr.zkClient, topicAndPartition.topic, brokerConfig.uncleanLeaderElectionEnable)) To understand how the broker dynamically loads per topic configs, you can look at TopicConfigManager. It registers a zookeeper listener on the topic config change path and atomically switches the log config object to reflect the new per topic config overrides. > On Feb. 4, 2014, 11:21 p.m., Neha Narkhede wrote: > > core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala, line 64 > > <https://reviews.apache.org/r/17537/diff/3/?file=456705#file456705line64> > > > > the config object should already have the per topic preference for > > unclean leader election. So we don't have to read from zookeeper again. > > Andrew Olson wrote: > It doesn't look like this is actually the case. The KafkaConfig is passed > from the KafkaServer to the KafkaController with no topic context, and the > controller does not appear to be integrated with the topic log configuration > logic in the TopicConfigManager/LogManager. > > Just to confirm my understanding of the code, I removed this Zookeeper > read and doing so caused the two TopicOverride integration tests that I added > to fail. Is there is a simpler or less awkward way to implement this as per > topic configuration? Reading the config on demand from ZK seems like the > simplest and least invasive option since this should not be a frequently > executed code path, but I could be missing something obvious. >> It doesn't look like this is actually the case. The KafkaConfig is passed >> from the KafkaServer to the KafkaController with no topic context, and the >> controller does not appear to be integrated with the topic log configuration >> logic in the TopicConfigManager/LogManager. To understand how the broker dynamically loads per topic configs, you can look at TopicConfigManager. It registers a zookeeper listener on the topic config change path and atomically switches the log config object to reflect the new per topic config overrides. I haven't looked at the tests in detail, but are you introducing the per topic config overrides the same way as TopicCommand does (by writing to the correct zk path)? That is the only way it will be automatically reloaded by all the brokers, including the controller - Neha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17537/#review33658 ----------------------------------------------------------- On Jan. 30, 2014, 7:45 p.m., Andrew Olson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17537/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2014, 7:45 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1028 > https://issues.apache.org/jira/browse/KAFKA-1028 > > > Repository: kafka > > > Description > ------- > > KAFKA-1028: per topic configuration of preference for consistency over > availability > > > Diffs > ----- > > core/src/main/scala/kafka/admin/AdminUtils.scala > a167756f0fd358574c8ccb42c5c96aaf13def4f5 > core/src/main/scala/kafka/common/NoReplicaOnlineException.scala > a1e12794978adf79020936c71259bbdabca8ee68 > core/src/main/scala/kafka/controller/KafkaController.scala > a0267ae2670e8d5f365e49ec0fa5db1f62b815bf > core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala > fd9200f3bf941aab54df798bb5899eeb552ea3a3 > core/src/main/scala/kafka/log/LogConfig.scala > 0b32aeeffcd9d4755ac90573448d197d3f729749 > core/src/main/scala/kafka/server/KafkaConfig.scala > 3c3aafc2b3f06fc8f3168a8a9c1e0b08e944c1ef > core/src/main/scala/kafka/server/ReplicaFetcherThread.scala > 73e605eb31bc71642d48b0bb8bd1632fd70b9dca > core/src/test/scala/unit/kafka/integration/RollingBounceTest.scala > b585f0ec0b1c402d95a3b34934dab7545dcfcb1f > core/src/test/scala/unit/kafka/integration/UncleanLeaderElectionTest.scala > PRE-CREATION > core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala > 89c207a3f56c7a7711f8cee6fb277626329882a6 > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 426b1a7bea1d83a64081f2c6b672c88c928713b7 > > Diff: https://reviews.apache.org/r/17537/diff/ > > > Testing > ------- > > > Thanks, > > Andrew Olson > >
