----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24621/#review50500 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/errors/TopicExistsException.java <https://reviews.apache.org/r/24621/#comment88323> This comment is misleading since the exception complains about an existing topic. Also, not sure why this should be a RetriableException since the only way to get out of this is to delete the topic, which is an admin operation. clients/src/main/java/org/apache/kafka/common/errors/UnableToCreateTopicException.java <https://reviews.apache.org/r/24621/#comment88324> How about renaming this to TopicCreationFailedException? It would also be helpful for the comment to provide details about which kind of failures can occur during topic creation. clients/src/main/java/org/apache/kafka/common/protocol/Errors.java <https://reviews.apache.org/r/24621/#comment88385> The doc here is incorrect. clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java <https://reviews.apache.org/r/24621/#comment88337> The doc string for "topics" can be improved -> "The list of topics to be created" clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java <https://reviews.apache.org/r/24621/#comment88339> same here core/src/main/scala/kafka/common/UnableToCreateTopicException.scala <https://reviews.apache.org/r/24621/#comment88329> Why is this one required if we already have one of these defined under clients? core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/24621/#comment88383> we cannot have metadata request handling access zookeeper. In the past, we've gotten bitten by the kind of slowdown the zk access causes. Besides, I think this check is not required. In the case that the metadata cache is not updated but the topic is in zookeeper, we could just return UnknownTopicOrPartitionCode and explicitly state that create topic is not a sync operation. core/src/main/scala/kafka/server/KafkaApis.scala <https://reviews.apache.org/r/24621/#comment88387> Also I think we have to maintain the old config behavior for backwards compatibility and prevent making any changes to the old producer. We can auto.create off by default. That way folks using the old producer can continue using auto create as is. And those using new producer will have it turned off. I'm guessing we have to maintain this behavior at least for 1-2 releases. core/src/main/scala/kafka/tools/GetOffsetShell.scala <https://reviews.apache.org/r/24621/#comment88384> Same comment as in SimpleConsumerShell core/src/main/scala/kafka/tools/SimpleConsumerShell.scala <https://reviews.apache.org/r/24621/#comment88334> I think there are 2 kinds of error messages that could be useful depending on whether the topic exists or the topic metadata was incomplete/incorrect. I think it is useful to give an explicit error message saying "The topic %s doesn't exist" in case the error code is UnknownTopic... - Neha Narkhede On Aug. 13, 2014, 1:09 a.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24621/ > ----------------------------------------------------------- > > (Updated Aug. 13, 2014, 1:09 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1507 > https://issues.apache.org/jira/browse/KAFKA-1507 > > > Repository: kafka > > > Description > ------- > > KAFKA-1507. Using GetOffsetShell against non-existent topic creates the > topic unintentionally. Added CreateTopicRequest to the protocol , removed > create topic from TopicMetadataRequest. > > > Diffs > ----- > > clients/src/main/java/org/apache/kafka/clients/NetworkClient.java > eea270abb16f40c9f3b47c4ea96be412fb4fdc8b > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > f58b8508d3f813a51015abed772c704390887d7e > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > f9de4af426449cceca12a8de9a9f54a6241d28d8 > > clients/src/main/java/org/apache/kafka/common/errors/TopicExistsException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/UnableToCreateTopicException.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java > 109fc965e09b2ed186a073351bd037ac8af20a4c > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > 3374bd98be8e565608c4e764ed10afdae383fb6f > clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java > 7517b879866fc5dad5f8d8ad30636da8bbe7784a > > clients/src/main/java/org/apache/kafka/common/requests/CreateTopicRequest.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/requests/CreateTopicResponse.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java > 1a55242e9399fa4669630b55110d530f954e1279 > > clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java > df37fc6d8f0db0b8192a948426af603be3444da4 > core/src/main/scala/kafka/api/CreateTopicRequest.scala PRE-CREATION > core/src/main/scala/kafka/api/CreateTopicResponse.scala PRE-CREATION > core/src/main/scala/kafka/api/CreateTopicResult.scala PRE-CREATION > core/src/main/scala/kafka/api/RequestKeys.scala > c24c0345feedc7b9e2e9f40af11bfa1b8d328c43 > core/src/main/scala/kafka/client/ClientUtils.scala > ce7ede3f6d60e756e252257bd8c6fedc21f21e1c > core/src/main/scala/kafka/common/ErrorMapping.scala > 5559d26ba2b96059f719754a351fa4598ca8a70b > core/src/main/scala/kafka/common/UnableToCreateTopicException.scala > PRE-CREATION > core/src/main/scala/kafka/javaapi/CreateTopicRequest.scala PRE-CREATION > core/src/main/scala/kafka/javaapi/CreateTopicResponse.scala PRE-CREATION > core/src/main/scala/kafka/javaapi/CreateTopicResult.scala PRE-CREATION > core/src/main/scala/kafka/producer/BrokerPartitionInfo.scala > 13a8aa665016e19284c8ced35bac2195fe9e4378 > core/src/main/scala/kafka/producer/ProducerConfig.scala > 3cdf23dce3407f1770b9c6543e3a8ae8ab3ff255 > core/src/main/scala/kafka/producer/SyncProducer.scala > 489f0077512d9a69be81649c490274964290fa40 > core/src/main/scala/kafka/server/KafkaApis.scala > bb94673c8f3079a4053e0a7d58689e69775cdf8b > core/src/main/scala/kafka/tools/GetOffsetShell.scala > 9c6064e201eebbcd5b276a0dedd02937439edc94 > core/src/main/scala/kafka/tools/SimpleConsumerShell.scala > 36314f412a8281aece2789fd2b74a106b82c57d2 > core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala > 789e74c776d99f204e0ef87eba1f55a3551abdf2 > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > cd16ced5465d098be7a60498326b2a98c248f343 > > core/src/test/scala/unit/kafka/consumer/ZookeeperConsumerConnectorTest.scala > e1d87112a2a587aa3a2f5875f278b276c32f45ac > core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala > 35dc071b1056e775326981573c9618d8046e601d > core/src/test/scala/unit/kafka/producer/ProducerTest.scala > dd71d81041e1baa3e3ebdbc38f60f247ebd65948 > core/src/test/scala/unit/kafka/utils/TestUtils.scala > c4e13c5240c8303853d08cc3b40088f8c7dae460 > > Diff: https://reviews.apache.org/r/24621/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
