> On March 14, 2014, 8:13 p.m., Jay Kreps wrote: > > This is great! > > > > I don't think we should add a special purpose method that hard codes one > > property in AdminUtils, I think the helper code is actually there in > > LogConfig. > > > > I see Neha's point about adding a higher-level config to log config but I > > think it is worth it and not so bad as it is just a config--there is no > > major code coupling problem. > > > > The only real question I have which is really for Neha is whether adding > > these new ZK queries is a performance concern for leadership election and > > transfer? Should instead the controller or whoever just watch and > > constantly replicate these log configs all the time so that when election > > time comes no new fetches are required. This could perhaps be done as a > > follow-up improvement... > > > > > > Andrew Olson wrote: > Thanks for the feedback. The LogConfig.fromProps(...) approach works, but > it looks like I'd need to update the implementation to provide default values > for each of the props.getProperty(...) calls (it currently assumes that all > supported properties are included in the provided Properties). Should I go > ahead and make that change? > > Neha Narkhede wrote: > I do think that the zookeeper read is a performance concern during > leadership changes, since the patch currently does one read per partition. I > think it is not so much a problem with the patch but more with us improving > our handling of per topic non-log configs. As you suggested, we should add > the ability to refresh per topic configs on the controller and possibly the > ReplicaManager as well. But I think we can do that as a follow up improvement > and accept this patch. > > Neha Narkhede wrote: > >> Thanks for the feedback. The LogConfig.fromProps(...) approach works, > but it looks like I'd need to update the implementation to provide default > values for each of the props.getProperty(...) calls (it currently assumes > that all supported properties are included in the provided Properties). > Should I go ahead and make that change? > > Yes please. Would you mind updating your patch with that change and also > rebasing it? I think we can check it in after that.
Patch has been rebased and updated. - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17537/#review37265 ----------------------------------------------------------- On March 17, 2014, 2:39 p.m., Andrew Olson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17537/ > ----------------------------------------------------------- > > (Updated March 17, 2014, 2:39 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/common/NoReplicaOnlineException.scala > a1e12794978adf79020936c71259bbdabca8ee68 > core/src/main/scala/kafka/controller/KafkaController.scala > 5db24a73a62c725a76c79936abb15b1fda3b770b > core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala > fa29bbe82db35551f43ac487912fba7bae1b2599 > core/src/main/scala/kafka/log/LogConfig.scala > 18c86fed5efc765f9b059775988cc83ef0ef3c3b > core/src/main/scala/kafka/server/KafkaConfig.scala > d07796ec87fa9bf0d13e1690240b85979aba3224 > 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 > 772d2140ed926a2f9f0c99aea60daf1a9b987073 > > Diff: https://reviews.apache.org/r/17537/diff/ > > > Testing > ------- > > > Thanks, > > Andrew Olson > >
