> 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.
>> 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. - Neha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17537/#review37265 ----------------------------------------------------------- On March 4, 2014, 12:48 a.m., Andrew Olson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17537/ > ----------------------------------------------------------- > > (Updated March 4, 2014, 12:48 a.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 > 36ddeb44490e8343a4e8056c45726ac660e4b2f9 > core/src/main/scala/kafka/common/NoReplicaOnlineException.scala > a1e12794978adf79020936c71259bbdabca8ee68 > core/src/main/scala/kafka/controller/KafkaController.scala > b58cdcd16ffb62ba5329b8b2776f2bd18440b3a0 > core/src/main/scala/kafka/controller/PartitionLeaderSelector.scala > fa29bbe82db35551f43ac487912fba7bae1b2599 > core/src/main/scala/kafka/log/LogConfig.scala > 0b32aeeffcd9d4755ac90573448d197d3f729749 > core/src/main/scala/kafka/server/KafkaConfig.scala > 04a5d39be3c41f5cb3589d95d0bd0f4fb4d7030d > core/src/main/scala/kafka/server/ReplicaFetcherThread.scala > 73e605eb31bc71642d48b0bb8bd1632fd70b9dca > core/src/test/scala/unit/kafka/admin/AdminTest.scala > d5644ea40ec7678b975c4775546b79fcfa9f64b7 > 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 > >
