----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19957/#review39840 -----------------------------------------------------------
Thanks for the patch. Some additional comments. 1. The introduction of MetadataCache is a good idea and provides better code isolation. 1.1 Could we move aliveBroker in it too? 1.2 Could we make it a private class? 1.3 Could we more the readwriteLock and methods that access the lock inside this class too? This way, the usage of the lock is only limited inside the class. 2. I am concerned of the overhead of the lock in ensureTopicExists(). This method is called in every produce and fetch request and is a global lock. I think the purpose is to disable access to a topic when a topic is deleted. However, even if we don't call ensureTopicExists(), once a replica is deleted by the ReplicaManager, all requests will get the UnknownTopicOrPartitonException. ReplicaManager may find out that a partition is being deleted a bit later. But this is not a big issue since we just accumulated a bit more data which will be deleted soon. - Jun Rao On April 8, 2014, 8:38 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19957/ > ----------------------------------------------------------- > > (Updated April 8, 2014, 8:38 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1356 > https://issues.apache.org/jira/browse/KAFKA-1356 > > > Repository: kafka > > > Description > ------- > > KAFKA-1356 Improve topic metadata handling in kafka api > > > Diffs > ----- > > core/src/main/scala/kafka/api/TopicMetadata.scala > 0513a59ed94e556894b3515dc38666ee9a66ae3d > core/src/main/scala/kafka/controller/KafkaController.scala > c8c02ced543a6278ba5b1edfa73a370f1edb1891 > core/src/main/scala/kafka/server/KafkaApis.scala > 705d87ed874f2363e6d9d6ea1143bc6035a0a9d6 > core/src/test/scala/unit/kafka/admin/AdminTest.scala > d5644ea40ec7678b975c4775546b79fcfa9f64b7 > core/src/test/scala/unit/kafka/server/SimpleFetchTest.scala > 22bb6f2847b8895f8fbba6c531963ebb0fffe2ca > core/src/test/scala/unit/kafka/utils/TestUtils.scala > 2054c25bbced6bd90c092a1974975732ad346146 > > Diff: https://reviews.apache.org/r/19957/diff/ > > > Testing > ------- > > > Thanks, > > Timothy Chen > >
