You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Joel Koshy <jj...@gmail.com> on 2014/07/17 21:47:03 UTC
Review Request 23655: Provide alternate (round-robin-style) rebalance
algorithms.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
Review request for kafka.
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description
-------
tweaks
Ready to submit
Diffs
-----
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing
-------
Thanks,
Joel Koshy
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review48454
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/ConsumerConfig.scala
<https://reviews.apache.org/r/23655/#comment85261>
Will do.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment85262>
Will fix.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment85265>
After thinking about it and discussing off-thread, we are thinking of:
def allocate(partitions, consumerIdsPerTopic)
At the same time, this code is not particularly complex so reusability is nice-have - worst case we can just adapt it to use in the new consumer.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment85268>
Thanks for catching that - will fix.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment85269>
After thinking about it I think we can actually merge the two and get rid of symmetric. So we will just have "range" and "roundrobin" (or maybe name it "uniform"). Will see how that turns out after I update the patch.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment85074>
This uses the "<" operator defined in StringOps - so it probably translates to compareTo underneath the hood. That said, I can change it to compareTo just to be sure.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment85075>
Yeah I didn't bother with describing the old allocator as it was copied/moved from the consumer connector - I will add brief comments.
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/23655/#comment85271>
Will do.
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/23655/#comment85077>
I think it is very useful. While the offset checker is useful, it can take a while to run if you want to (say) get a count of all partitions owned by the consumer. An mbean on the other hand helps continuously monitor how even your partitions are distributed across all your consumers.
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala
<https://reviews.apache.org/r/23655/#comment85272>
Will add it.
- Joel Koshy
On July 18, 2014, 10:57 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 10:57 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The updated diff contains the mbeans for ownership counts.
> The comments in the code and the summary are pretty self-explanatory.
>
> Things to think about:
> * Naming - do symmetric/range/roundrobin make sense?
> * The comments briefly summarize why we needed a separate symmetric mode but let me know if that is unclear.
> * Rebalance time will be slightly higher - I have not measured (will do that)
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala a20ab90165cc7ebb1cf44078efe23a53938c8df6
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review48319
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment84791>
will be within
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment84795>
Should this be RoundRobin, and the "RoundRobin" be Symmetric?
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment84794>
Is String "<" well defined in all JVMs? Shall we use str1.compareTo(str2)?
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment84796>
Comments for this logic?
- Guozhang Wang
On July 18, 2014, 10:57 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 10:57 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The updated diff contains the mbeans for ownership counts.
> The comments in the code and the summary are pretty self-explanatory.
>
> Things to think about:
> * Naming - do symmetric/range/roundrobin make sense?
> * The comments briefly summarize why we needed a separate symmetric mode but let me know if that is unclear.
> * Rebalance time will be slightly higher - I have not measured (will do that)
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala a20ab90165cc7ebb1cf44078efe23a53938c8df6
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review48165
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/ConsumerConfig.scala
<https://reviews.apache.org/r/23655/#comment84474>
In the new consumer config, we call this partition.assignment.strategy. It would be good if we make them consistent.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment84475>
License header.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment84946>
Could we make this api a bit more general so that we can reuse it in the new consumer? I was thinking of sth like
def allocate(partitions: List[TopicPartition], consumers: List[ConsumerItem])
case class ConsumerItem(consumerId: String, subscribedTopics: List[String])
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment84948>
Not sure if we need both Symmetric and round-robin, if we order the list of consumers by sth like their hashcode. It would also be useful to add an example in the comment that illustrates how the assignment works.
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/23655/#comment84949>
Perhaps we can keep the name and the corresponding allocation instance in sth like a PartitionAllocatorFactory?
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/23655/#comment84950>
Do we need this metric? Would the ConsumerOffsetChecker be suffice?
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala
<https://reviews.apache.org/r/23655/#comment84953>
License header.
- Jun Rao
On July 18, 2014, 10:57 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated July 18, 2014, 10:57 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The updated diff contains the mbeans for ownership counts.
> The comments in the code and the summary are pretty self-explanatory.
>
> Things to think about:
> * Naming - do symmetric/range/roundrobin make sense?
> * The comments briefly summarize why we needed a separate symmetric mode but let me know if that is unclear.
> * Rebalance time will be slightly higher - I have not measured (will do that)
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala a20ab90165cc7ebb1cf44078efe23a53938c8df6
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 27
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line27>
> >
> > Could we add a comment on what the return value is, especially the String part?
Sure - done
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 44
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line44>
> >
> > Could we use map { case(topic, replicaAssignment) => }?
Will do.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 57
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line57>
> >
> > owned => subscribed ?
Yes - that's right.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 77
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line77>
> >
> > Is the comment still valid? It doesn't seem we maintain isLocal any more.
It is obsolete - will fix.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 82
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line82>
> >
> > It seems that we are reading the consumer info from ZK multiple times. Could we do all the necessary zk reads in AllocatorContext once? Then allocate() is just doing the allocation.
Yes I did realize this while implementing it. i.e., we have to read ZK multiple times - once for each consumer. We could do it in the context, but I felt it was almost identical. That said, I think what you are saying is more for separation of functionality - i.e., the allocator should only allocate given some context. Let me see if I can make that work - the allocation context may be slightly different between the two allocators but I think that's okay.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, line 85
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line85>
> >
> > Could we avoid using tuples and use named values through case instead?
will do, although I won't be changing the RHS since that actually makes the code unnecessarily verbose. it will be clearer in the patch.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, lines 104-105
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line104>
> >
> > Should this be info?
Yes it should be info.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala, lines 50-51
> > <https://reviews.apache.org/r/23655/diff/3/?file=664713#file664713line50>
> >
> > If we move all ZK accesses outside of allocate(), the test can also be simplified.
That is true - I actually tried it and it is simpler but not a whole lot. Furthermore changing the API as I mentioned earlier results in some changes in the range allocator which I don't want to make in this patch. In other words, I would rather remove the test than change the API in order to simplify it.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAllocator.scala, lines 154-155
> > <https://reviews.apache.org/r/23655/diff/3/?file=664708#file664708line154>
> >
> > Could we just use consumersPerTopicMap(topic)?
Yes - although this is just moved from ZookeeperConsumerConnector. I did not want to touch the range allocation code.
> On Aug. 20, 2014, 3:23 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/utils/ZkUtils.scala, lines 661-665
> > <https://reviews.apache.org/r/23655/diff/3/?file=664712#file664712line661>
> >
> > Should this method be in TopicCount?
Due to the allocation context change above I think we can get rid of this. Let me see.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51104
-----------------------------------------------------------
On Aug. 19, 2014, 7:08 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2014, 7:08 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-687; Add a roundrobin partition assignment scheme to the consumer.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51104
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89062>
Could we add a comment on what the return value is, especially the String part?
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89063>
Could we use map { case(topic, replicaAssignment) => }?
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89064>
owned => subscribed ?
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89068>
Is the comment still valid? It doesn't seem we maintain isLocal any more.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89066>
It seems that we are reading the consumer info from ZK multiple times. Could we do all the necessary zk reads in AllocatorContext once? Then allocate() is just doing the allocation.
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89065>
Could we avoid using tuples and use named values through case instead?
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89067>
Should this be info?
core/src/main/scala/kafka/consumer/PartitionAllocator.scala
<https://reviews.apache.org/r/23655/#comment89061>
Could we just use consumersPerTopicMap(topic)?
core/src/main/scala/kafka/utils/ZkUtils.scala
<https://reviews.apache.org/r/23655/#comment89060>
Should this method be in TopicCount?
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala
<https://reviews.apache.org/r/23655/#comment89069>
If we move all ZK accesses outside of allocate(), the test can also be simplified.
- Jun Rao
On Aug. 19, 2014, 7:08 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2014, 7:08 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-687; Add a roundrobin partition assignment scheme to the consumer.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51019
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/ConsumerConfig.scala
<https://reviews.apache.org/r/23655/#comment88899>
will get rid of symmetric
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala
<https://reviews.apache.org/r/23655/#comment88901>
I'll fix this obsolete comment as well.
- Joel Koshy
On Aug. 19, 2014, 7:08 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2014, 7:08 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> KAFKA-687; Add a roundrobin partition assignment scheme to the consumer.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51153
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89191>
AssignmentContext now contains all the zk reads, so the assignor only does assignment. This potentially makes it easier to unit test since you don't need to mock.
HOWEVER, I actually think it is easier to use the zkclient mock - because the code to build the threadId map, consumers for topic map, etc. is fairly complex and is already in ZkUtils/elsewhere. So that's why I did not change the unit tests.
With that said, let me know if anyone feels it is better to revert to the previous version.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89190>
FYI, this code is repetitive, but cannot avoid it easily since "this" needs to be the first statement. I would rather not pollute ZkUtils any further, so I thought it was okay to leave as is even though it incurs a redundant zk read.
Or we can just remove it and avoid the current primary constructor of this class which was added to attempt to avoid the mock classes in the unit test. (See other comment also)
- Joel Koshy
On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 1:10 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v4
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Guozhang Wang <gu...@linkedin.com>.
> On Aug. 23, 2014, 9:57 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 159
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line159>
> >
> > It may be more helpful to log the partition results in the end than logging the starting of the partition at the beginning.
>
> Joel Koshy wrote:
> We should avoid that because it will be a mostly useless extremely long line. Mirror makers could (for example) mirror tens of thousands of partitions.
With the printing of the curPartitions we are already generating a large log entry for MM right? What I was suggesting is to move this entry to the end along with the assignment result result. This will not effectively add more lines to the logging entry, but maybe more informative.
- Guozhang
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51344
-----------------------------------------------------------
On Aug. 25, 2014, 7:36 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2014, 7:36 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v5
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java d97962d3840179b1abf01459522c58e59102ac8d
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 23, 2014, 9:57 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 159
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line159>
> >
> > It may be more helpful to log the partition results in the end than logging the starting of the partition at the beginning.
We should avoid that because it will be a mostly useless extremely long line. Mirror makers could (for example) mirror tens of thousands of partitions.
> On Aug. 23, 2014, 9:57 p.m., Guozhang Wang wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 74
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line74>
> >
> > I cannot understand a): what does "stream" of a topic-partition mean here? Should that be "each consumer instance have the same number of streams for each subscribed topic"?
Yeah I'm not sure if we have a consistent terminology here. This is similar to num-streams. Say if there is a subscription of the form tA: 2 then tA will receive two streams. Some call it thread-ids some call it streams. Let me know if you have a better wording for this.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51344
-----------------------------------------------------------
On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 1:10 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v4
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51344
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/ConsumerConfig.scala
<https://reviews.apache.org/r/23655/#comment89559>
Remove "symmetric"
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89556>
I cannot understand a): what does "stream" of a topic-partition mean here? Should that be "each consumer instance have the same number of streams for each subscribed topic"?
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89557>
It may be more helpful to log the partition results in the end than logging the starting of the partition at the beginning.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89558>
maybe " * "?
- Guozhang Wang
On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 1:10 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v4
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51432
-----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java
<https://reviews.apache.org/r/23655/#comment89749>
(ignore this particular change)
- Joel Koshy
On Aug. 25, 2014, 7:36 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2014, 7:36 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v5
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java d97962d3840179b1abf01459522c58e59102ac8d
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 28, 2014, 1:32 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ConsumerConfig.scala, line 52
> > <https://reviews.apache.org/r/23655/diff/5/?file=668390#file668390line52>
> >
> > This probably should be named DefaultPartitionAssignmentStrategy.
Ok - it seems a number of defaults are improperly named.
> On Aug. 28, 2014, 1:32 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 47
> > <https://reviews.apache.org/r/23655/diff/5/?file=668391#file668391line47>
> >
> > partitionAssginment is a bit confusing now that we have the partition assignor. Perhaps both the val and the method name in ZkUtils should be called replicaAssignmentPerTopic.
Agreed - there is already another replicaAssignmentPerTopic utility in ZkUtils, but it turns out I can actually use ZkUtils.getPartitionsForTopic.
> On Aug. 28, 2014, 1:32 a.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala, lines 214-222
> > <https://reviews.apache.org/r/23655/diff/5/?file=668397#file668397line214>
> >
> > I am not sure if this is necessary. The input is a map with topicAndPartition as the key. This implies that each partition has only 1 owner.
That's very true - dumb mistake. I actually do want to check for conflicts so I refactored the test a bit and managed to get rid of the "forConsumer" API version of the roundrobin assignor.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51694
-----------------------------------------------------------
On Aug. 25, 2014, 7:36 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2014, 7:36 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v5
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java d97962d3840179b1abf01459522c58e59102ac8d
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 28, 2014, 1:32 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala, line 721
> > <https://reviews.apache.org/r/23655/diff/5/?file=668393#file668393line721>
> >
> > Could we use case to avoid _._1 ?
> >
> > Also, could you explain why we want to call .view on a map?
I probably won't put a comment in the code since it is standard scala. But the view helps delay the materialization of the collection until actually needed/forced either explicitly or some forcing operation. I did some toy tests with the above collection to verify.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51694
-----------------------------------------------------------
On Aug. 25, 2014, 7:36 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2014, 7:36 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v5
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java d97962d3840179b1abf01459522c58e59102ac8d
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51694
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/ConsumerConfig.scala
<https://reviews.apache.org/r/23655/#comment90241>
This probably should be named DefaultPartitionAssignmentStrategy.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment90246>
partitionAssginment is a bit confusing now that we have the partition assignor. Perhaps both the val and the method name in ZkUtils should be called replicaAssignmentPerTopic.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment90252>
topic-partition => topic?
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/23655/#comment90261>
Could we use case to avoid _._1 ?
Also, could you explain why we want to call .view on a map?
core/src/main/scala/kafka/tools/MirrorMaker.scala
<https://reviews.apache.org/r/23655/#comment90262>
This is probably not intended.
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment90263>
invalid comment
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment90264>
I am not sure if this is necessary. The input is a map with topicAndPartition as the key. This implies that each partition has only 1 owner.
- Jun Rao
On Aug. 25, 2014, 7:36 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2014, 7:36 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v5
>
>
> Diffs
> -----
>
> clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java d97962d3840179b1abf01459522c58e59102ac8d
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51884
-----------------------------------------------------------
Ship it!
Thanks for the patch. +1 with the following minor comments.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment90562>
Could we make it clear that this method only returns the partition assginment for the consumer instance specified in ctx?
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment90563>
It probably would be clearer if we specify the type of each of those vals.
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment90557>
The comment is not accurate since uniqueness is verfied above.
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment90558>
No need to reference PartitionAssignorTest.
- Jun Rao
On Aug. 28, 2014, 11:20 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 28, 2014, 11:20 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v6
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
(Updated Aug. 28, 2014, 11:20 p.m.)
Review request for kafka.
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description (updated)
-------
v6
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing
-------
* I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
Thanks,
Joel Koshy
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
(Updated Aug. 25, 2014, 7:36 p.m.)
Review request for kafka.
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description (updated)
-------
v5
Diffs (updated)
-----
clients/src/main/java/org/apache/kafka/common/requests/MetadataResponse.java d97962d3840179b1abf01459522c58e59102ac8d
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
core/src/main/scala/kafka/tools/MirrorMaker.scala b8698ee1469c8fbc92ccc176d916eb3e28b87867
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing
-------
* I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
Thanks,
Joel Koshy
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 23, 2014, 10:03 p.m., Guozhang Wang wrote:
> > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala, lines 85-86
> > <https://reviews.apache.org/r/23655/diff/4/?file=665829#file665829line85>
> >
> > Can we use a more meaningful name here, e.g. consumerIndex?
I'm actually removing this altogether. (See comments to/from Jun).
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51345
-----------------------------------------------------------
On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 1:10 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v4
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Guozhang Wang <gu...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51345
-----------------------------------------------------------
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment89561>
Can we use a more meaningful name here, e.g. consumerIndex?
- Guozhang Wang
On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 1:10 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v4
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 75
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line75>
> >
> > It seems that (a) implies (b).
Not really - within a consumer instance we need each topic to have the same number of streams. However, the number of streams can be different on another consumer instance. The intent of the constraint is that any partition can be assigned to any of the available thread-id's on a given consumer instance.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 83
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line83>
> >
> > Are those fields needed in the contructor? The same set of fields are already passed into AssignmentContext.
Can be removed.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, lines 104-120
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line104>
> >
> > One of the implications of this is that the distribution is going to be sensitive to # topics. In the case when #topics == #consumerThreads, partitions from the same topic will be assigned to the same consumerThread. In general, if #topics is a multiple of #consumerThreads, a similar issue can happen.
> >
> > So, I am wondering if this is better than just sorting the consumer ids in some random order like hashcode. For debugging purpose, we can log the consumer ids in that order.
As discussed, we could but I'm not sure if we should. I tried it earlier for other reasons so I have that code as well. It makes it much harder to predict the final assigment though.
That said most operations folks want to have a uniform distribution so I can go back to using that sort version.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, lines 105-106
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line105>
> >
> > Since RoundRobin assigns partitions across topics, should we just log all partitions and all consumers once in the right order?
We should probably avoid logging everything in one line (although I agree that logging the sort order would have been useful). There could be tens of thousands of partitions so it would be an extremely long line. So I would prefer to stick with this (line per-topic).
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 154
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line154>
> >
> > Could we do partitionsForTopic(topic) here too?
Yes.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala, lines 85-86
> > <https://reviews.apache.org/r/23655/diff/4/?file=665829#file665829line85>
> >
> > Do we need a while loop here? Can this just be 2 + random.nextInt(consumerCount - 1)?
I will remove the consistency assertion across consumers.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala, lines 260-263
> > <https://reviews.apache.org/r/23655/diff/4/?file=665829#file665829line260>
> >
> > Shouldn't we assert count==1? Do we need to also make sure every original partition is in assignment?
Yes.
Also, the check for every partition being in the assignment is above this (givenPartitions == assignedPartitions)
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala, line 289
> > <https://reviews.apache.org/r/23655/diff/4/?file=665829#file665829line289>
> >
> > Should we rename this to partitionsPerStream?
Sure - will rename to partitionCountPerStream
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 88
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line88>
> >
> > Instead of passing in forConsumers, would it be simpler to just return the assignment for each consumer in a map? The unit test can also be made simpler since we just need to test the coverage and uniformity.
We could, but it seems a bit unwieldy to do that - i.e., having to maintain yet another map of map... and in the consumer connector we only want the assignment for that consumer id. Also, I ended up removing the "conflict" test from the unit test so it is simpler.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/main/scala/kafka/consumer/PartitionAssignor.scala, line 53
> > <https://reviews.apache.org/r/23655/diff/4/?file=665824#file665824line53>
> >
> > To avoid duplicating the ZK read, we can probably change all the vals in the constructor to def. Then, we can intialize some private vals during initialization.
Actually, I reverted to the old form because we ended up not needing the current form in the unit test.
> On Aug. 22, 2014, 4:46 p.m., Jun Rao wrote:
> > core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala, lines 88-124
> > <https://reviews.apache.org/r/23655/diff/4/?file=665829#file665829line88>
> >
> > This test is pretty complicated. I am wondering if we need to test the case that c1 and cx give consistent assignment. If the inputs to the assignor are the same and the assignor is deterministic, the assignment is always going to be consistent. We probably just need to test completeness and uniqueness.
See above.
- Joel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51182
-----------------------------------------------------------
On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 1:10 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v4
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Jun Rao <ju...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review51182
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89203>
To avoid duplicating the ZK read, we can probably change all the vals in the constructor to def. Then, we can intialize some private vals during initialization.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89207>
It seems that (a) implies (b).
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89314>
Are those fields needed in the contructor? The same set of fields are already passed into AssignmentContext.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89315>
Instead of passing in forConsumers, would it be simpler to just return the assignment for each consumer in a map? The unit test can also be made simpler since we just need to test the coverage and uniformity.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89205>
One of the implications of this is that the distribution is going to be sensitive to # topics. In the case when #topics == #consumerThreads, partitions from the same topic will be assigned to the same consumerThread. In general, if #topics is a multiple of #consumerThreads, a similar issue can happen.
So, I am wondering if this is better than just sorting the consumer ids in some random order like hashcode. For debugging purpose, we can log the consumer ids in that order.
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89431>
Since RoundRobin assigns partitions across topics, should we just log all partitions and all consumers once in the right order?
core/src/main/scala/kafka/consumer/PartitionAssignor.scala
<https://reviews.apache.org/r/23655/#comment89204>
Could we do partitionsForTopic(topic) here too?
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment89302>
Do we need a while loop here? Can this just be 2 + random.nextInt(consumerCount - 1)?
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment89432>
This test is pretty complicated. I am wondering if we need to test the case that c1 and cx give consistent assignment. If the inputs to the assignor are the same and the assignor is deterministic, the assignment is always going to be consistent. We probably just need to test completeness and uniqueness.
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment89434>
Shouldn't we assert count==1? Do we need to also make sure every original partition is in assignment?
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala
<https://reviews.apache.org/r/23655/#comment89436>
Should we rename this to partitionsPerStream?
- Jun Rao
On Aug. 21, 2014, 1:10 a.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2014, 1:10 a.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> v4
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
> core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
(Updated Aug. 21, 2014, 1:10 a.m.)
Review request for kafka.
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description (updated)
-------
v4
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAssignor.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAssignorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing
-------
* I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
Thanks,
Joel Koshy
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
(Updated Aug. 19, 2014, 7:08 p.m.)
Review request for kafka.
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description (updated)
-------
KAFKA-687; Add a roundrobin partition assignment scheme to the consumer.
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala 8b0ae5785e08272d0ea12483beae597f2eac4343
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala acfd064bdba2b031f8869011da79649efd80946f
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala 00df4621fd724826a1e79d849c762ac1c4f49868
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing
-------
* I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
Thanks,
Joel Koshy
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
(Updated July 18, 2014, 10:57 p.m.)
Review request for kafka.
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description (updated)
-------
The updated diff contains the mbeans for ownership counts.
The comments in the code and the summary are pretty self-explanatory.
Things to think about:
* Naming - do symmetric/range/roundrobin make sense?
* The comments briefly summarize why we needed a separate symmetric mode but let me know if that is unclear.
* Rebalance time will be slightly higher - I have not measured (will do that)
Diffs
-----
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala a20ab90165cc7ebb1cf44078efe23a53938c8df6
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing
-------
* I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
Thanks,
Joel Koshy
Re: Review Request 23655: Patch for KAFKA-687
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
(Updated July 18, 2014, 10:55 p.m.)
Review request for kafka.
Summary (updated)
-----------------
Patch for KAFKA-687
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description (updated)
-------
tweaks
Ready to submit
Add owner count gauges.
Diffs (updated)
-----
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
core/src/main/scala/kafka/metrics/KafkaMetricsGroup.scala a20ab90165cc7ebb1cf44078efe23a53938c8df6
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing
-------
* I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
Thanks,
Joel Koshy
Re: Review Request 23655: Provide alternate (round-robin-style) rebalance
algorithms.
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/#review48043
-----------------------------------------------------------
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala
<https://reviews.apache.org/r/23655/#comment84294>
(FYI: while this is being reviewed I will look into adding some mbeans to report topic-level and total owner counts for the consumer.)
- Joel Koshy
On July 17, 2014, 7:50 p.m., Joel Koshy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23655/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 7:50 p.m.)
>
>
> Review request for kafka.
>
>
> Bugs: KAFKA-687
> https://issues.apache.org/jira/browse/KAFKA-687
>
>
> Repository: kafka
>
>
> Description
> -------
>
> The comments in the code and the summary are pretty self-explanatory.
>
> Things to think about:
> * Naming - do symmetric/range/roundrobin make sense?
> * The comments briefly summarize why we needed a separate symmetric mode but let me know if that is unclear.
> * Rebalance time will be slightly higher - I have not measured (will do that)
> * I would like to add some mbeans to show ownership counts.
>
>
> Diffs
> -----
>
> core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
> core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
> core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
> core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
> core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
> core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/23655/diff/
>
>
> Testing
> -------
>
> * I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
>
>
> Thanks,
>
> Joel Koshy
>
>
Re: Review Request 23655: Provide alternate (round-robin-style) rebalance
algorithms.
Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23655/
-----------------------------------------------------------
(Updated July 17, 2014, 7:50 p.m.)
Review request for kafka.
Bugs: KAFKA-687
https://issues.apache.org/jira/browse/KAFKA-687
Repository: kafka
Description (updated)
-------
The comments in the code and the summary are pretty self-explanatory.
Things to think about:
* Naming - do symmetric/range/roundrobin make sense?
* The comments briefly summarize why we needed a separate symmetric mode but let me know if that is unclear.
* Rebalance time will be slightly higher - I have not measured (will do that)
* I would like to add some mbeans to show ownership counts.
Diffs
-----
core/src/main/scala/kafka/consumer/ConsumerConfig.scala 1cf2f62ba02e4aa66bfa7575865e5d57baf82212
core/src/main/scala/kafka/consumer/PartitionAllocator.scala PRE-CREATION
core/src/main/scala/kafka/consumer/TopicCount.scala c79311097c5bd6718cb6a7fc403f804a1a939353
core/src/main/scala/kafka/consumer/ZookeeperConsumerConnector.scala 65f518d47c7555c42c4bff39c211814831f4b8b6
core/src/main/scala/kafka/utils/ZkUtils.scala dcdc1ce2b02c996294e19cf480736106aaf29511
core/src/test/scala/unit/kafka/consumer/PartitionAllocatorTest.scala PRE-CREATION
Diff: https://reviews.apache.org/r/23655/diff/
Testing (updated)
-------
* I did the unit tests (including the new one) as well as mirror maker system test suite with roundrobin. While this is being reviewed I will run the system tests with symmetric
Thanks,
Joel Koshy