You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Guozhang Wang <wa...@gmail.com> on 2015/10/05 20:51:35 UTC
Re: Review Request 36858: Patch for KAFKA-2120
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36858/#review101511
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java (line 427)
<https://reviews.apache.org/r/36858/#comment158932>
One minor thing: could you remove this comment?
- Guozhang Wang
On Sept. 28, 2015, 11:16 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36858/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2015, 11:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2120
> https://issues.apache.org/jira/browse/KAFKA-2120
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Solved compile error
>
>
> Addressed Jason's comments for Kip-19
>
>
> Addressed Jun's comments
>
>
> Addressed Jason's comments about the default values for requestTimeout
>
>
> checkpoint
>
>
> Addressed Joel's concerns. Also tried to include Jun's feedback.
>
>
> Fixed a minor comment
>
>
> Solved unittest issue
>
>
> Addressed Jun's comments regarding NetworkClient
>
>
> Addressed Jun's comments about disconnect() in Selector
>
>
> changed logging level to debug
>
>
> Addressed Joels comments to break out early from the loop while aborting expired batches
>
>
> Addressed Jun's comments
>
>
> Addressed Jason's concern about iterating over timeout request in getNodesWithTimedOutRequest()
>
>
> Addressed Joel's comments
>
>
> Changed the default timeouts for requesttimeout in KafkaConfig and also made requestrtimeout consistent in KafkaConsumer
>
>
> Fixed the forward referencing bug in KafkaConfig.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java dc8f0f115bcda893c95d17c0a57be8d14518d034
> clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java 7d24c6f5dd2b63b96584f3aa8922a1d048dc1ae4
> clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java 15d00d4e484bb5d51a9ae6857ed6e024a2cc1820
> clients/src/main/java/org/apache/kafka/clients/KafkaClient.java f46c0d9b5eb73887c62a0e09c96e9d8c964c709d
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 51a6c5dc47c4008e92b9fdd2ee359b573184e8ed
> clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java b9a2d4e2bc565f0ee72b27791afe5c894af262f1
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 3ac2be840c5d313a497ab67dc024d1578d4679fd
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java 9517d9d0cd480d5ba1d12f1fde7963e60528d2f8
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 804d569498396d431880641041fc9292076452cb
> clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 06f00a99a73a288df9afa8c1d4abe3580fa968a6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/BufferPool.java 4cb1e50d6c4ed55241aeaef1d3af09def5274103
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java a152bd7697dca55609a9ec4cfe0a82c10595fbc3
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 06182db1c3a5da85648199b4c0c98b80ea7c6c0c
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java d2e64f7cd8bf56e433a210905b2874f71eee9ea0
> clients/src/main/java/org/apache/kafka/common/network/Selectable.java 70e74bd6aa629c430b2850ca40c97df0b16e5d75
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 5a4909e4e5afe1b7ce2c33b9324d3947d2d91a38
> clients/src/test/java/org/apache/kafka/clients/MockClient.java e5815f56bdf8e2d980f2bc36b831ed234c0ac781
> clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java 69c93c3adf674b1640534c3d7410fcaafaf2232c
> clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java 2c693824fa53db1e38766b8c66a0ef42ef9d0f3a
> clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java 5b2e4ffaeab7127648db608c179703b27b577414
> clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java aa44991777a855f4b7f4f7bf17107c69393ff8ff
> clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java df1205c935bee9a30a50816dbade64d6014b1ef2
> clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 3a684d98b05cadfb25c6f7f9a038ef1f6697edbf
> clients/src/test/java/org/apache/kafka/test/MockSelector.java f83fd9b794a3bd191121a22bcb40fd6ec31d83b5
> core/src/main/scala/kafka/controller/ControllerChannelManager.scala b1cf668dbb5bc8617d320fd3a307c6c8005b78a2
> core/src/main/scala/kafka/server/KafkaConfig.scala 1e8b2331486ffe55bfcc0919e48e12aad23b7d3c
> core/src/main/scala/kafka/server/KafkaServer.scala f3f1fa653b9c85e729cbe32b63669011ada46547
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 6c85e52e172bfc0278e724068a48a271bc15dc9f
> core/src/main/scala/kafka/tools/ProducerPerformance.scala 46a68e97b8bcc8821f21e4220ce9b3acedc5dafe
> core/src/main/scala/kafka/utils/NetworkClientBlockingOps.scala ad10721de844725f27a116611209992cea61b088
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 1198df02ddd7727269e84a751ba99520f6d5584a
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 5b4f2db4607ae6d17696c1140f1a771ce75c80e0
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 09b8444c2add87f0f70dbb182e892977a6b5c243
>
> Diff: https://reviews.apache.org/r/36858/diff/
>
>
> Testing
> -------
>
> Unit test pass
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36858: Patch for KAFKA-2120
Posted by Ismael Juma <mi...@juma.me.uk>.
> On Oct. 5, 2015, 6:51 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 427
> > <https://reviews.apache.org/r/36858/diff/14/?file=1086222#file1086222line427>
> >
> > One minor thing: could you remove this comment?
I had asked for that to be removed too (you can see it in the discussion), but Mayuresh said that some more work needed to be done before it could be removed (I didn't quite understand it to be honest).
- Ismael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36858/#review101511
-----------------------------------------------------------
On Sept. 28, 2015, 11:16 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36858/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2015, 11:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2120
> https://issues.apache.org/jira/browse/KAFKA-2120
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Solved compile error
>
>
> Addressed Jason's comments for Kip-19
>
>
> Addressed Jun's comments
>
>
> Addressed Jason's comments about the default values for requestTimeout
>
>
> checkpoint
>
>
> Addressed Joel's concerns. Also tried to include Jun's feedback.
>
>
> Fixed a minor comment
>
>
> Solved unittest issue
>
>
> Addressed Jun's comments regarding NetworkClient
>
>
> Addressed Jun's comments about disconnect() in Selector
>
>
> changed logging level to debug
>
>
> Addressed Joels comments to break out early from the loop while aborting expired batches
>
>
> Addressed Jun's comments
>
>
> Addressed Jason's concern about iterating over timeout request in getNodesWithTimedOutRequest()
>
>
> Addressed Joel's comments
>
>
> Changed the default timeouts for requesttimeout in KafkaConfig and also made requestrtimeout consistent in KafkaConsumer
>
>
> Fixed the forward referencing bug in KafkaConfig.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java dc8f0f115bcda893c95d17c0a57be8d14518d034
> clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java 7d24c6f5dd2b63b96584f3aa8922a1d048dc1ae4
> clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java 15d00d4e484bb5d51a9ae6857ed6e024a2cc1820
> clients/src/main/java/org/apache/kafka/clients/KafkaClient.java f46c0d9b5eb73887c62a0e09c96e9d8c964c709d
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 51a6c5dc47c4008e92b9fdd2ee359b573184e8ed
> clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java b9a2d4e2bc565f0ee72b27791afe5c894af262f1
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 3ac2be840c5d313a497ab67dc024d1578d4679fd
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java 9517d9d0cd480d5ba1d12f1fde7963e60528d2f8
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 804d569498396d431880641041fc9292076452cb
> clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 06f00a99a73a288df9afa8c1d4abe3580fa968a6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/BufferPool.java 4cb1e50d6c4ed55241aeaef1d3af09def5274103
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java a152bd7697dca55609a9ec4cfe0a82c10595fbc3
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 06182db1c3a5da85648199b4c0c98b80ea7c6c0c
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java d2e64f7cd8bf56e433a210905b2874f71eee9ea0
> clients/src/main/java/org/apache/kafka/common/network/Selectable.java 70e74bd6aa629c430b2850ca40c97df0b16e5d75
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 5a4909e4e5afe1b7ce2c33b9324d3947d2d91a38
> clients/src/test/java/org/apache/kafka/clients/MockClient.java e5815f56bdf8e2d980f2bc36b831ed234c0ac781
> clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java 69c93c3adf674b1640534c3d7410fcaafaf2232c
> clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java 2c693824fa53db1e38766b8c66a0ef42ef9d0f3a
> clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java 5b2e4ffaeab7127648db608c179703b27b577414
> clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java aa44991777a855f4b7f4f7bf17107c69393ff8ff
> clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java df1205c935bee9a30a50816dbade64d6014b1ef2
> clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 3a684d98b05cadfb25c6f7f9a038ef1f6697edbf
> clients/src/test/java/org/apache/kafka/test/MockSelector.java f83fd9b794a3bd191121a22bcb40fd6ec31d83b5
> core/src/main/scala/kafka/controller/ControllerChannelManager.scala b1cf668dbb5bc8617d320fd3a307c6c8005b78a2
> core/src/main/scala/kafka/server/KafkaConfig.scala 1e8b2331486ffe55bfcc0919e48e12aad23b7d3c
> core/src/main/scala/kafka/server/KafkaServer.scala f3f1fa653b9c85e729cbe32b63669011ada46547
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 6c85e52e172bfc0278e724068a48a271bc15dc9f
> core/src/main/scala/kafka/tools/ProducerPerformance.scala 46a68e97b8bcc8821f21e4220ce9b3acedc5dafe
> core/src/main/scala/kafka/utils/NetworkClientBlockingOps.scala ad10721de844725f27a116611209992cea61b088
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 1198df02ddd7727269e84a751ba99520f6d5584a
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 5b4f2db4607ae6d17696c1140f1a771ce75c80e0
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 09b8444c2add87f0f70dbb182e892977a6b5c243
>
> Diff: https://reviews.apache.org/r/36858/diff/
>
>
> Testing
> -------
>
> Unit test pass
>
>
> Thanks,
>
> Mayuresh Gharat
>
>
Re: Review Request 36858: Patch for KAFKA-2120
Posted by Mayuresh Gharat <gh...@gmail.com>.
> On Oct. 5, 2015, 6:51 p.m., Guozhang Wang wrote:
> > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java, line 427
> > <https://reviews.apache.org/r/36858/diff/14/?file=1086222#file1086222line427>
> >
> > One minor thing: could you remove this comment?
>
> Ismael Juma wrote:
> I had asked for that to be removed too (you can see it in the discussion), but Mayuresh said that some more work needed to be done before it could be removed (I didn't quite understand it to be honest).
We have a ticket KAFKA-2428 which needs to be handled after KIP-19.
The reason for that ticket is availble on KAFKA-2120 Jira.
This was the brief discussion on the ticket KAFKA-2120:
The request time is for exception handling purpose, the request timeout should be the highest timeout across all the timeout. Can we just sanity check the configurations user provides? The rules should be:
Request timeout > session timeout.
Request timeout > fetch.max.wait.timeout
request timeout won't kick in before the other timeout reached.
The sanity check patch should be relatively simple. We can either do it in this patch or in a separate patch.
I created KAFKA-2428 to handle this.
I have explained about discrepency of long/int datatype of requestTimeout in kafkaConsumer and sanity checks on Ismael's comments above. Probably I should have listed the Jira ticket there for the sanity checks which would have made this more clear.
The TODO was left as it is to remind us that there is still work needed to be done after KIP-19 for KafkaConsumer on usage of requestTimeout.
- Mayuresh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36858/#review101511
-----------------------------------------------------------
On Sept. 28, 2015, 11:16 p.m., Mayuresh Gharat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36858/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2015, 11:16 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-2120
> https://issues.apache.org/jira/browse/KAFKA-2120
>
>
> Repository: kafka
>
>
> Description
> -------
>
> Solved compile error
>
>
> Addressed Jason's comments for Kip-19
>
>
> Addressed Jun's comments
>
>
> Addressed Jason's comments about the default values for requestTimeout
>
>
> checkpoint
>
>
> Addressed Joel's concerns. Also tried to include Jun's feedback.
>
>
> Fixed a minor comment
>
>
> Solved unittest issue
>
>
> Addressed Jun's comments regarding NetworkClient
>
>
> Addressed Jun's comments about disconnect() in Selector
>
>
> changed logging level to debug
>
>
> Addressed Joels comments to break out early from the loop while aborting expired batches
>
>
> Addressed Jun's comments
>
>
> Addressed Jason's concern about iterating over timeout request in getNodesWithTimedOutRequest()
>
>
> Addressed Joel's comments
>
>
> Changed the default timeouts for requesttimeout in KafkaConfig and also made requestrtimeout consistent in KafkaConsumer
>
>
> Fixed the forward referencing bug in KafkaConfig.
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/clients/ClientRequest.java dc8f0f115bcda893c95d17c0a57be8d14518d034
> clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java 7d24c6f5dd2b63b96584f3aa8922a1d048dc1ae4
> clients/src/main/java/org/apache/kafka/clients/InFlightRequests.java 15d00d4e484bb5d51a9ae6857ed6e024a2cc1820
> clients/src/main/java/org/apache/kafka/clients/KafkaClient.java f46c0d9b5eb73887c62a0e09c96e9d8c964c709d
> clients/src/main/java/org/apache/kafka/clients/NetworkClient.java 51a6c5dc47c4008e92b9fdd2ee359b573184e8ed
> clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java b9a2d4e2bc565f0ee72b27791afe5c894af262f1
> clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java 3ac2be840c5d313a497ab67dc024d1578d4679fd
> clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerNetworkClient.java 9517d9d0cd480d5ba1d12f1fde7963e60528d2f8
> clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java 804d569498396d431880641041fc9292076452cb
> clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java 06f00a99a73a288df9afa8c1d4abe3580fa968a6
> clients/src/main/java/org/apache/kafka/clients/producer/internals/BufferPool.java 4cb1e50d6c4ed55241aeaef1d3af09def5274103
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java a152bd7697dca55609a9ec4cfe0a82c10595fbc3
> clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordBatch.java 06182db1c3a5da85648199b4c0c98b80ea7c6c0c
> clients/src/main/java/org/apache/kafka/clients/producer/internals/Sender.java d2e64f7cd8bf56e433a210905b2874f71eee9ea0
> clients/src/main/java/org/apache/kafka/common/network/Selectable.java 70e74bd6aa629c430b2850ca40c97df0b16e5d75
> clients/src/main/java/org/apache/kafka/common/network/Selector.java 5a4909e4e5afe1b7ce2c33b9324d3947d2d91a38
> clients/src/test/java/org/apache/kafka/clients/MockClient.java e5815f56bdf8e2d980f2bc36b831ed234c0ac781
> clients/src/test/java/org/apache/kafka/clients/NetworkClientTest.java 69c93c3adf674b1640534c3d7410fcaafaf2232c
> clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java 2c693824fa53db1e38766b8c66a0ef42ef9d0f3a
> clients/src/test/java/org/apache/kafka/clients/producer/internals/RecordAccumulatorTest.java 5b2e4ffaeab7127648db608c179703b27b577414
> clients/src/test/java/org/apache/kafka/clients/producer/internals/SenderTest.java aa44991777a855f4b7f4f7bf17107c69393ff8ff
> clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java df1205c935bee9a30a50816dbade64d6014b1ef2
> clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java 3a684d98b05cadfb25c6f7f9a038ef1f6697edbf
> clients/src/test/java/org/apache/kafka/test/MockSelector.java f83fd9b794a3bd191121a22bcb40fd6ec31d83b5
> core/src/main/scala/kafka/controller/ControllerChannelManager.scala b1cf668dbb5bc8617d320fd3a307c6c8005b78a2
> core/src/main/scala/kafka/server/KafkaConfig.scala 1e8b2331486ffe55bfcc0919e48e12aad23b7d3c
> core/src/main/scala/kafka/server/KafkaServer.scala f3f1fa653b9c85e729cbe32b63669011ada46547
> core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 6c85e52e172bfc0278e724068a48a271bc15dc9f
> core/src/main/scala/kafka/tools/ProducerPerformance.scala 46a68e97b8bcc8821f21e4220ce9b3acedc5dafe
> core/src/main/scala/kafka/utils/NetworkClientBlockingOps.scala ad10721de844725f27a116611209992cea61b088
> core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 1198df02ddd7727269e84a751ba99520f6d5584a
> core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala 5b4f2db4607ae6d17696c1140f1a771ce75c80e0
> core/src/test/scala/unit/kafka/utils/TestUtils.scala 09b8444c2add87f0f70dbb182e892977a6b5c243
>
> Diff: https://reviews.apache.org/r/36858/diff/
>
>
> Testing
> -------
>
> Unit test pass
>
>
> Thanks,
>
> Mayuresh Gharat
>
>