----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20190/#review40012 -----------------------------------------------------------
Thanks for the patch. Some comments below. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/20190/#comment72799> Could we make this class private? core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/20190/#comment72795> Since we don't hold the lock while checking the cache, there is no memory barrier crossed and there is no guarantee that we will see the current value in the cache. So, I don't see the value of calling ensureTopicExists. Plus, ensureTopicExists is needed only as an optimization, not for correctness. I recommend that we remove the check on ensureTopicExists. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/20190/#comment72798> I am not sure that we should just blindly set the controllerEpoch in ReplicaManager. ReplicaManager is supposed to hold the latest controller epoch so that it can disregard requests sent from an older controller. So, we only need to update controllerEpoch if the new value is higher than the current one. Perhaps we can add a util function in ReplicaManager to do that. - Jun Rao On April 10, 2014, 1:48 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20190/ > ----------------------------------------------------------- > > (Updated April 10, 2014, 1:48 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1356 > https://issues.apache.org/jira/browse/KAFKA-1356 > > > Repository: kafka > > > Description > ------- > > Refactor metadata cache code in kafka api and remove lock on ensureTopicExists > > > Diffs > ----- > > core/src/main/scala/kafka/server/KafkaApis.scala > d96229e2d4aa7006b0dbd81055ce5a2459d8758c > > Diff: https://reviews.apache.org/r/20190/diff/ > > > Testing > ------- > > > Thanks, > > Timothy Chen > >
