Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated April 7, 2015, 3:41 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs (updated) - core/src/main/scala/kafka/server/AbstractFetcherThread.scala f178527048ea5dbc8c1fde81a5bac54a8e1634c6 core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 2d84afa451277e3769368678fd8ea578a8a81774 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
On March 24, 2015, 10:46 p.m., Guozhang Wang wrote:
core/src/main/scala/kafka/server/AbstractFetcherThread.scala, lines 81-86
https://reviews.apache.org/r/31366/diff/2/?file=898415#file898415line81
Jun has a comment about the case when all partitions gets inactive,
which is common when the fetched broker has been just gone through leader
migration.
We can move the foreach statement before the if statement, and after
foreach check if any partitions gets added, if not just backoff for
fetchBackoffMs.
Sriharsha Chintalapani wrote:
Thanks for the review. Are you looking at something like this. This
wouldn't handle if we have partitionMap populated but all of them are
inactive.
partitionMap.foreach {
case((topicAndPartition, partitionFetchState)) =
if(partitionFetchState.isActive)
fetchRequestBuilder.addFetch(topicAndPartition.topic,
topicAndPartition.partition,
partitionFetchState.offset, fetchSize)
}
if (partitionMap.isEmpty)
partitionMapCond.await(fetchBackOffMs, TimeUnit.MILLISECONDS)
or do we want to check if all the currentParttions are inactive and than
backoff? that would be expensive to check if all the partitions or active or
not in dowork.
Guozhang Wang wrote:
What I think is a bit different and maybe simpler:
For FetchRequestBuilder, in the build() call its requestMap will be
cleared after the fetch request is created, so we can just add another
function in FetchRequestBuilder return boolean indicating if its request map
is empty. With this we can get rid of the allPartitionsInactive flag.
Thanks for the suggestion. I used fetchRequest.requestInfo.isEmpty which we
already have. If the fetchRequestBuilder.partitionMap is empty than the
requestInfo will also be empty. Can you please take a look at the latest diff.
- Sriharsha
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31366/#review77674
---
On April 7, 2015, 3:41 p.m., Sriharsha Chintalapani wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31366/
---
(Updated April 7, 2015, 3:41 p.m.)
Review request for kafka.
Bugs: KAFKA-1461
https://issues.apache.org/jira/browse/KAFKA-1461
Repository: kafka
Description
---
KAFKA-1461. Replica fetcher thread does not implement any back-off behavior.
KAFKA-1461. Replica fetcher thread does not implement any back-off behavior.
KAFKA-1461. Replica fetcher thread does not implement any back-off behavior.
Diffs
-
core/src/main/scala/kafka/server/AbstractFetcherThread.scala
f178527048ea5dbc8c1fde81a5bac54a8e1634c6
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala
2d84afa451277e3769368678fd8ea578a8a81774
Diff: https://reviews.apache.org/r/31366/diff/
Testing
---
Thanks,
Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/#review79205 --- Ship it! LGTM, Jun do you want to take another look? - Guozhang Wang On April 7, 2015, 3:41 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated April 7, 2015, 3:41 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs - core/src/main/scala/kafka/server/AbstractFetcherThread.scala f178527048ea5dbc8c1fde81a5bac54a8e1634c6 core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 2d84afa451277e3769368678fd8ea578a8a81774 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
On March 24, 2015, 10:46 p.m., Guozhang Wang wrote:
core/src/main/scala/kafka/server/AbstractFetcherThread.scala, lines 81-86
https://reviews.apache.org/r/31366/diff/2/?file=898415#file898415line81
Jun has a comment about the case when all partitions gets inactive,
which is common when the fetched broker has been just gone through leader
migration.
We can move the foreach statement before the if statement, and after
foreach check if any partitions gets added, if not just backoff for
fetchBackoffMs.
Sriharsha Chintalapani wrote:
Thanks for the review. Are you looking at something like this. This
wouldn't handle if we have partitionMap populated but all of them are
inactive.
partitionMap.foreach {
case((topicAndPartition, partitionFetchState)) =
if(partitionFetchState.isActive)
fetchRequestBuilder.addFetch(topicAndPartition.topic,
topicAndPartition.partition,
partitionFetchState.offset, fetchSize)
}
if (partitionMap.isEmpty)
partitionMapCond.await(fetchBackOffMs, TimeUnit.MILLISECONDS)
or do we want to check if all the currentParttions are inactive and than
backoff? that would be expensive to check if all the partitions or active or
not in dowork.
What I think is a bit different and maybe simpler:
For FetchRequestBuilder, in the build() call its requestMap will be cleared
after the fetch request is created, so we can just add another function in
FetchRequestBuilder return boolean indicating if its request map is empty. With
this we can get rid of the allPartitionsInactive flag.
- Guozhang
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31366/#review77674
---
On April 4, 2015, 3:48 a.m., Sriharsha Chintalapani wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31366/
---
(Updated April 4, 2015, 3:48 a.m.)
Review request for kafka.
Bugs: KAFKA-1461
https://issues.apache.org/jira/browse/KAFKA-1461
Repository: kafka
Description
---
KAFKA-1461. Replica fetcher thread does not implement any back-off behavior.
KAFKA-1461. Replica fetcher thread does not implement any back-off behavior.
KAFKA-1461. Replica fetcher thread does not implement any back-off behavior.
Diffs
-
core/src/main/scala/kafka/server/AbstractFetcherThread.scala
5d5cf5897cc37b3595f14bfe9d7cde43456bcc4b
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala
96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0
Diff: https://reviews.apache.org/r/31366/diff/
Testing
---
Thanks,
Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated April 4, 2015, 3:48 a.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description (updated) --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs (updated) - core/src/main/scala/kafka/server/AbstractFetcherThread.scala 5d5cf5897cc37b3595f14bfe9d7cde43456bcc4b core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated March 27, 2015, 10:31 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs (updated) - core/src/main/scala/kafka/server/AbstractFetcherThread.scala 5d5cf5897cc37b3595f14bfe9d7cde43456bcc4b core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/#review78120 --- Couple of comments. Thanks! core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment126590 Can you add a comment explaning why this flag is used? It only seems to be used in doWork() so perhaps it shouldn't be a member variable? core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment126589 I guess you don't need to set this to false if it already is. core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment126594 Perhaps a trace log here before awaiting? core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment126602 Can you add some documentation for this class? - Aditya Auradkar On March 27, 2015, 10:31 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated March 27, 2015, 10:31 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs - core/src/main/scala/kafka/server/AbstractFetcherThread.scala 5d5cf5897cc37b3595f14bfe9d7cde43456bcc4b core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
On March 27, 2015, 10:52 p.m., Aditya Auradkar wrote: core/src/main/scala/kafka/server/AbstractFetcherThread.scala, line 276 https://reviews.apache.org/r/31366/diff/3/?file=908482#file908482line276 Can you add some documentation for this class? Thanks for the review. Updated the patch as per your suggestion. - Sriharsha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/#review78120 --- On March 27, 2015, 11:56 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated March 27, 2015, 11:56 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs - core/src/main/scala/kafka/server/AbstractFetcherThread.scala 5d5cf5897cc37b3595f14bfe9d7cde43456bcc4b core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated March 27, 2015, 11:56 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs (updated) - core/src/main/scala/kafka/server/AbstractFetcherThread.scala 5d5cf5897cc37b3595f14bfe9d7cde43456bcc4b core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated March 28, 2015, 12:02 a.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description (updated) --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs (updated) - core/src/main/scala/kafka/server/AbstractFetcherThread.scala 5d5cf5897cc37b3595f14bfe9d7cde43456bcc4b core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/#review77674 --- core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment125847 Jun has a comment about the case when all partitions gets inactive, which is common when the fetched broker has been just gone through leader migration. We can move the foreach statement before the if statement, and after foreach check if any partitions gets added, if not just backoff for fetchBackoffMs. - Guozhang Wang On March 17, 2015, 11:03 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated March 17, 2015, 11:03 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs - core/src/main/scala/kafka/server/AbstractFetcherThread.scala e731df4b2a3e44aa3d761713a09b1070aff81430 core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
On March 24, 2015, 10:46 p.m., Guozhang Wang wrote:
core/src/main/scala/kafka/server/AbstractFetcherThread.scala, lines 81-86
https://reviews.apache.org/r/31366/diff/2/?file=898415#file898415line81
Jun has a comment about the case when all partitions gets inactive,
which is common when the fetched broker has been just gone through leader
migration.
We can move the foreach statement before the if statement, and after
foreach check if any partitions gets added, if not just backoff for
fetchBackoffMs.
Thanks for the review. Are you looking at something like this. This wouldn't
handle if we have partitionMap populated but all of them are inactive.
partitionMap.foreach {
case((topicAndPartition, partitionFetchState)) =
if(partitionFetchState.isActive)
fetchRequestBuilder.addFetch(topicAndPartition.topic,
topicAndPartition.partition,
partitionFetchState.offset, fetchSize)
}
if (partitionMap.isEmpty)
partitionMapCond.await(fetchBackOffMs, TimeUnit.MILLISECONDS)
or do we want to check if all the currentParttions are inactive and than
backoff? that would be expensive to check if all the partitions or active or
not in dowork.
- Sriharsha
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31366/#review77674
---
On March 17, 2015, 11:03 p.m., Sriharsha Chintalapani wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31366/
---
(Updated March 17, 2015, 11:03 p.m.)
Review request for kafka.
Bugs: KAFKA-1461
https://issues.apache.org/jira/browse/KAFKA-1461
Repository: kafka
Description
---
KAFKA-1461. Replica fetcher thread does not implement any back-off behavior.
Diffs
-
core/src/main/scala/kafka/server/AbstractFetcherThread.scala
e731df4b2a3e44aa3d761713a09b1070aff81430
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala
96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0
Diff: https://reviews.apache.org/r/31366/diff/
Testing
---
Thanks,
Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated March 17, 2015, 11:03 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs (updated) - core/src/main/scala/kafka/server/AbstractFetcherThread.scala e731df4b2a3e44aa3d761713a09b1070aff81430 core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 96faa7b4ed7c9ba8a3f6f9f114bd94e19b3a7ac0 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/#review74866 --- core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment121680 OffsetAndDelay / OffsetAndState is a bit confusing, maybe we can just use PartitionFetchState? core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment121689 It seems we do not need to pass in the OffsetAndDelay object here as we will create new one anyways. We can still pass in Long, and with that OffsetAndDelay is just internal to AbstractFetcherThread. core/src/main/scala/kafka/server/OffsetAndDelay.scala https://reviews.apache.org/r/31366/#comment121685 Maybe we can just put this case class into AbstractFetcherThread and expose to AbstractFetcherManager. core/src/main/scala/kafka/server/ReplicaFetcherThread.scala https://reviews.apache.org/r/31366/#comment121686 Are these imports necessary? core/src/main/scala/kafka/server/ReplicaFetcherThread.scala https://reviews.apache.org/r/31366/#comment121688 Is this intentional? - Guozhang Wang On Feb. 24, 2015, 6:02 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated Feb. 24, 2015, 6:02 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs - core/src/main/scala/kafka/server/AbstractFetcherManager.scala 20c00cb8cc2351950edbc8cb1752905a0c26e79f core/src/main/scala/kafka/server/AbstractFetcherThread.scala 8c281d4668f92eff95a4a5df3c03c4b5b20e7095 core/src/main/scala/kafka/server/KafkaConfig.scala 14bf3216bae030331bdf76b3266ed0e73526c3de core/src/main/scala/kafka/server/OffsetAndDelay.scala PRE-CREATION core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 6879e730282185bda3d6bc3659cb15af0672cecf core/src/test/scala/unit/kafka/server/ReplicaFetchTest.scala da4bafc1e2a94a436efe395aab1888fc21e55748 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 31366: Patch for KAFKA-1461
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/#review74160 --- core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment120683 Using the foreach method on partitionMap.get(topicAndPartition) would accomplish the same thing (lines 114-116, and 164) without the need for pattern matching. core/src/main/scala/kafka/server/AbstractFetcherThread.scala https://reviews.apache.org/r/31366/#comment120684 Again, foreach would be more idomatic, or take advantage of already being in a for loop. - Eric Olander On Feb. 24, 2015, 6:02 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31366/ --- (Updated Feb. 24, 2015, 6:02 p.m.) Review request for kafka. Bugs: KAFKA-1461 https://issues.apache.org/jira/browse/KAFKA-1461 Repository: kafka Description --- KAFKA-1461. Replica fetcher thread does not implement any back-off behavior. Diffs - core/src/main/scala/kafka/server/AbstractFetcherManager.scala 20c00cb8cc2351950edbc8cb1752905a0c26e79f core/src/main/scala/kafka/server/AbstractFetcherThread.scala 8c281d4668f92eff95a4a5df3c03c4b5b20e7095 core/src/main/scala/kafka/server/KafkaConfig.scala 14bf3216bae030331bdf76b3266ed0e73526c3de core/src/main/scala/kafka/server/OffsetAndDelay.scala PRE-CREATION core/src/main/scala/kafka/server/ReplicaFetcherThread.scala 6879e730282185bda3d6bc3659cb15af0672cecf core/src/test/scala/unit/kafka/server/ReplicaFetchTest.scala da4bafc1e2a94a436efe395aab1888fc21e55748 Diff: https://reviews.apache.org/r/31366/diff/ Testing --- Thanks, Sriharsha Chintalapani
