Re: Review Request 23339: Patch for KAFKA-1507
On July 28, 2014, 3:16 a.m., Jun Rao wrote: clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java, lines 56-57 https://reviews.apache.org/r/23339/diff/3/?file=641061#file641061line56 We need to pass in the createTopic flag in this constructor too. In the producer, we will then set createTopic to true and the in the consumer, we will set it to true. I added another constructor public Metadata(long refreshBackoffMs, long metadataExpireMs, boolean createTopic) and kept this one as a backward compatibility if it causes confusion I'll remove this. I was thinking if the user is calling Metdata() from producer createTopic should be transparent to them as they expect to work since its part of broker config by default. - Sriharsha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/#review48822 --- On July 24, 2014, 12:07 a.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/ --- (Updated July 24, 2014, 12:07 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. Changes as per Jun's suggestions. Diffs - clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 444e69e7c95d5ffad19896fff0ab15cb4f5c9b4e clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java b22ca1dce65f665d84c2a980fd82f816e93d9960 clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 core/src/main/scala/kafka/api/TopicMetadataRequest.scala 7dca09ce637a40e125de05703dc42e8b611971ac core/src/main/scala/kafka/client/ClientUtils.scala ce7ede3f6d60e756e252257bd8c6fedc21f21e1c core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala b9e2bea7b442a19bcebd1b350d39541a8c9dd068 core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala b0b7be14d494ae8c87f4443b52db69d273c20316 core/src/main/scala/kafka/server/KafkaApis.scala fd5f12ee31e78cdcda8e24a0ab3e1740b3928884 core/src/main/scala/kafka/tools/GetOffsetShell.scala 9c6064e201eebbcd5b276a0dedd02937439edc94 core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala af4783646803e58714770c21f8c3352370f26854 core/src/main/scala/kafka/tools/SimpleConsumerShell.scala 36314f412a8281aece2789fd2b74a106b82c57d2 core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 1bf2667f47853585bc33ffb3e28256ec5f24ae84 core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 35dc071b1056e775326981573c9618d8046e601d Diff: https://reviews.apache.org/r/23339/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 23339: Patch for KAFKA-1507
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/#review48822 --- Thanks for the patch. A few more comments below. clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java https://reviews.apache.org/r/23339/#comment85560 We need to pass in the createTopic flag in this constructor too. In the producer, we will then set createTopic to true and the in the consumer, we will set it to true. clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java https://reviews.apache.org/r/23339/#comment85558 Yes, I agree with you that keeping the V0 constructor is a bit confusing and doesn't provide any value. Could you remove this and the V0 constructor OffsetCommitRequest as well? clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java https://reviews.apache.org/r/23339/#comment85557 Is this needed? core/src/main/scala/kafka/client/ClientUtils.scala https://reviews.apache.org/r/23339/#comment85559 I think we can just keep the original name and keep the default for createTopic to true to make it backward compatible. core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala https://reviews.apache.org/r/23339/#comment85561 To make this backward compatible, we need to keep the current constructor and add a new one with the createTopic flag. - Jun Rao On July 24, 2014, 12:07 a.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/ --- (Updated July 24, 2014, 12:07 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. Changes as per Jun's suggestions. Diffs - clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 444e69e7c95d5ffad19896fff0ab15cb4f5c9b4e clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java b22ca1dce65f665d84c2a980fd82f816e93d9960 clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 core/src/main/scala/kafka/api/TopicMetadataRequest.scala 7dca09ce637a40e125de05703dc42e8b611971ac core/src/main/scala/kafka/client/ClientUtils.scala ce7ede3f6d60e756e252257bd8c6fedc21f21e1c core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala b9e2bea7b442a19bcebd1b350d39541a8c9dd068 core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala b0b7be14d494ae8c87f4443b52db69d273c20316 core/src/main/scala/kafka/server/KafkaApis.scala fd5f12ee31e78cdcda8e24a0ab3e1740b3928884 core/src/main/scala/kafka/tools/GetOffsetShell.scala 9c6064e201eebbcd5b276a0dedd02937439edc94 core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala af4783646803e58714770c21f8c3352370f26854 core/src/main/scala/kafka/tools/SimpleConsumerShell.scala 36314f412a8281aece2789fd2b74a106b82c57d2 core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 1bf2667f47853585bc33ffb3e28256ec5f24ae84 core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 35dc071b1056e775326981573c9618d8046e601d Diff: https://reviews.apache.org/r/23339/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 23339: Patch for KAFKA-1507
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/ --- (Updated July 24, 2014, 12:07 a.m.) Review request for kafka. Bugs: KAFKA-1507 https://issues.apache.org/jira/browse/KAFKA-1507 Repository: kafka Description (updated) --- KAFKA-1507. Using GetOffsetShell against non-existent topic creates the topic unintentionally. Changes as per Jun's suggestions. Diffs (updated) - clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 444e69e7c95d5ffad19896fff0ab15cb4f5c9b4e clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java b22ca1dce65f665d84c2a980fd82f816e93d9960 clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 core/src/main/scala/kafka/api/TopicMetadataRequest.scala 7dca09ce637a40e125de05703dc42e8b611971ac core/src/main/scala/kafka/client/ClientUtils.scala ce7ede3f6d60e756e252257bd8c6fedc21f21e1c core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala b9e2bea7b442a19bcebd1b350d39541a8c9dd068 core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala b0b7be14d494ae8c87f4443b52db69d273c20316 core/src/main/scala/kafka/server/KafkaApis.scala fd5f12ee31e78cdcda8e24a0ab3e1740b3928884 core/src/main/scala/kafka/tools/GetOffsetShell.scala 9c6064e201eebbcd5b276a0dedd02937439edc94 core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala af4783646803e58714770c21f8c3352370f26854 core/src/main/scala/kafka/tools/SimpleConsumerShell.scala 36314f412a8281aece2789fd2b74a106b82c57d2 core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 1bf2667f47853585bc33ffb3e28256ec5f24ae84 core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 35dc071b1056e775326981573c9618d8046e601d Diff: https://reviews.apache.org/r/23339/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 23339: Patch for KAFKA-1507
On July 22, 2014, 6:52 p.m., Jun Rao wrote: core/src/main/scala/kafka/client/ClientUtils.scala, lines 86-87 https://reviews.apache.org/r/23339/diff/2/?file=639404#file639404line86 Perhaps this method can be named better since it may not just be for the consumer. Changed it to fetchTopicMetadataForNonProducer please let me know if you have a suggestion. - Sriharsha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/#review48397 --- On July 24, 2014, 12:07 a.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/ --- (Updated July 24, 2014, 12:07 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. Changes as per Jun's suggestions. Diffs - clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 444e69e7c95d5ffad19896fff0ab15cb4f5c9b4e clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java b22ca1dce65f665d84c2a980fd82f816e93d9960 clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 core/src/main/scala/kafka/api/TopicMetadataRequest.scala 7dca09ce637a40e125de05703dc42e8b611971ac core/src/main/scala/kafka/client/ClientUtils.scala ce7ede3f6d60e756e252257bd8c6fedc21f21e1c core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala b9e2bea7b442a19bcebd1b350d39541a8c9dd068 core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala b0b7be14d494ae8c87f4443b52db69d273c20316 core/src/main/scala/kafka/server/KafkaApis.scala fd5f12ee31e78cdcda8e24a0ab3e1740b3928884 core/src/main/scala/kafka/tools/GetOffsetShell.scala 9c6064e201eebbcd5b276a0dedd02937439edc94 core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala af4783646803e58714770c21f8c3352370f26854 core/src/main/scala/kafka/tools/SimpleConsumerShell.scala 36314f412a8281aece2789fd2b74a106b82c57d2 core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 1bf2667f47853585bc33ffb3e28256ec5f24ae84 core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 35dc071b1056e775326981573c9618d8046e601d Diff: https://reviews.apache.org/r/23339/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 23339: Patch for KAFKA-1507
On July 22, 2014, 6:52 p.m., Jun Rao wrote: Thanks for the patch and doing the rebasing. Some comments below. Thanks for the review. I've noticed NetworkClient.java always uses the latestVersion of an api. There doesn't look like the way I can send specific version of a request since the nextRequestHeader doesn't accept a version. Even if I am creating a version 0 of a request the requestheader will go with latest version of that particular request. Is there any reason to keep version as part of request header rather than inside of a request structure and we expect NetworkClient to use the latest version?. - Sriharsha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/#review48397 --- On July 24, 2014, 12:07 a.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/ --- (Updated July 24, 2014, 12:07 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. Changes as per Jun's suggestions. Diffs - clients/src/main/java/org/apache/kafka/clients/NetworkClient.java d8f9ce663ee24d2b0852c974136741280c39f8f8 clients/src/main/java/org/apache/kafka/clients/producer/internals/Metadata.java 4aa5b01d611631db72df47d50bbe30edb8c478db clients/src/main/java/org/apache/kafka/common/protocol/Protocol.java 7517b879866fc5dad5f8d8ad30636da8bbe7784a clients/src/main/java/org/apache/kafka/common/protocol/types/Struct.java 444e69e7c95d5ffad19896fff0ab15cb4f5c9b4e clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java b22ca1dce65f665d84c2a980fd82f816e93d9960 clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java df37fc6d8f0db0b8192a948426af603be3444da4 core/src/main/scala/kafka/api/TopicMetadataRequest.scala 7dca09ce637a40e125de05703dc42e8b611971ac core/src/main/scala/kafka/client/ClientUtils.scala ce7ede3f6d60e756e252257bd8c6fedc21f21e1c core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala b9e2bea7b442a19bcebd1b350d39541a8c9dd068 core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala b0b7be14d494ae8c87f4443b52db69d273c20316 core/src/main/scala/kafka/server/KafkaApis.scala fd5f12ee31e78cdcda8e24a0ab3e1740b3928884 core/src/main/scala/kafka/tools/GetOffsetShell.scala 9c6064e201eebbcd5b276a0dedd02937439edc94 core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala af4783646803e58714770c21f8c3352370f26854 core/src/main/scala/kafka/tools/SimpleConsumerShell.scala 36314f412a8281aece2789fd2b74a106b82c57d2 core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 1bf2667f47853585bc33ffb3e28256ec5f24ae84 core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 35dc071b1056e775326981573c9618d8046e601d Diff: https://reviews.apache.org/r/23339/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 23339: Patch for KAFKA-1507
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/ --- (Updated July 22, 2014, 5:27 p.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. Diffs (updated) - clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java b22ca1dce65f665d84c2a980fd82f816e93d9960 core/src/main/scala/kafka/api/TopicMetadataRequest.scala 7dca09ce637a40e125de05703dc42e8b611971ac core/src/main/scala/kafka/client/ClientUtils.scala ce7ede3f6d60e756e252257bd8c6fedc21f21e1c core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala b9e2bea7b442a19bcebd1b350d39541a8c9dd068 core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala b0b7be14d494ae8c87f4443b52db69d273c20316 core/src/main/scala/kafka/server/KafkaApis.scala fd5f12ee31e78cdcda8e24a0ab3e1740b3928884 core/src/main/scala/kafka/tools/GetOffsetShell.scala 9c6064e201eebbcd5b276a0dedd02937439edc94 core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala af4783646803e58714770c21f8c3352370f26854 core/src/main/scala/kafka/tools/SimpleConsumerShell.scala 36314f412a8281aece2789fd2b74a106b82c57d2 core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 1bf2667f47853585bc33ffb3e28256ec5f24ae84 core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 35dc071b1056e775326981573c9618d8046e601d Diff: https://reviews.apache.org/r/23339/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 23339: Patch for KAFKA-1507
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/#review48397 --- Thanks for the patch and doing the rebasing. Some comments below. clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java https://reviews.apache.org/r/23339/#comment84957 The request/response format for the new clients are defined in Protocol. So, we need to make the change there as well and bump up the version #. Then, we need to change this object accordingly. Se OffsetCommitRequest as an example. core/src/main/scala/kafka/api/TopicMetadataRequest.scala https://reviews.apache.org/r/23339/#comment84958 Does this need to be a short? Could it be just 1 byte? core/src/main/scala/kafka/client/ClientUtils.scala https://reviews.apache.org/r/23339/#comment84960 The client should just send request using the protocol of the latest version and it shouldn't need to know the version #. core/src/main/scala/kafka/client/ClientUtils.scala https://reviews.apache.org/r/23339/#comment84963 Perhaps this method can be named better since it may not just be for the consumer. - Jun Rao On July 22, 2014, 5:27 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/23339/ --- (Updated July 22, 2014, 5:27 p.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. Diffs - clients/src/main/java/org/apache/kafka/common/requests/MetadataRequest.java b22ca1dce65f665d84c2a980fd82f816e93d9960 core/src/main/scala/kafka/api/TopicMetadataRequest.scala 7dca09ce637a40e125de05703dc42e8b611971ac core/src/main/scala/kafka/client/ClientUtils.scala ce7ede3f6d60e756e252257bd8c6fedc21f21e1c core/src/main/scala/kafka/consumer/ConsumerFetcherManager.scala b9e2bea7b442a19bcebd1b350d39541a8c9dd068 core/src/main/scala/kafka/javaapi/TopicMetadataRequest.scala b0b7be14d494ae8c87f4443b52db69d273c20316 core/src/main/scala/kafka/server/KafkaApis.scala fd5f12ee31e78cdcda8e24a0ab3e1740b3928884 core/src/main/scala/kafka/tools/GetOffsetShell.scala 9c6064e201eebbcd5b276a0dedd02937439edc94 core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala af4783646803e58714770c21f8c3352370f26854 core/src/main/scala/kafka/tools/SimpleConsumerShell.scala 36314f412a8281aece2789fd2b74a106b82c57d2 core/src/test/scala/unit/kafka/admin/AddPartitionsTest.scala 1bf2667f47853585bc33ffb3e28256ec5f24ae84 core/src/test/scala/unit/kafka/integration/TopicMetadataTest.scala 35dc071b1056e775326981573c9618d8046e601d Diff: https://reviews.apache.org/r/23339/diff/ Testing --- Thanks, Sriharsha Chintalapani
