You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/05/08 16:01:38 UTC
[GitHub] [kafka] AshishRoyJava opened a new pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
AshishRoyJava opened a new pull request #8633:
URL: https://github.com/apache/kafka/pull/8633
This code change provides fix for the JIRA ticket KAFKA-9965.
Added synchronized block to safely decrement the counter on the map when a new batch is created.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] AshishRoyJava commented on a change in pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
AshishRoyJava commented on a change in pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#discussion_r422473147
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/RoundRobinPartitioner.java
##########
@@ -73,4 +73,12 @@ private int nextValue(String topic) {
public void close() {}
+ @Override
+ public void onNewBatch(String topic, Cluster cluster, int prevPartition) {
+ synchronized (topicCounterMap) {
Review comment:
@chia7712 Sure, will add a test for this and will try to replace this with computeIfPresent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] AshishRoyJava commented on a change in pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
AshishRoyJava commented on a change in pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#discussion_r422485633
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/RoundRobinPartitioner.java
##########
@@ -73,4 +73,12 @@ private int nextValue(String topic) {
public void close() {}
+ @Override
+ public void onNewBatch(String topic, Cluster cluster, int prevPartition) {
+ synchronized (topicCounterMap) {
Review comment:
@chia7712 I have replaced it with `computeIfPresent`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] AshishRoyJava commented on pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
AshishRoyJava commented on pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#issuecomment-626161764
> Nice finding! Could you add tests for this fix?
I was unable to modify the existing test methods. This `RoundRobinPartitioner#onNewBatch` is not being used anywhere in the existing test methods, hence there was no scope to check.
The `onNewBatch` overridden method only decrementing the counter in the map, so it needs to be tested from a method where it is being called.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] AshishRoyJava commented on pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
AshishRoyJava commented on pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#issuecomment-636624709
Any update here??
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#discussion_r422471952
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/RoundRobinPartitioner.java
##########
@@ -73,4 +73,12 @@ private int nextValue(String topic) {
public void close() {}
+ @Override
+ public void onNewBatch(String topic, Cluster cluster, int prevPartition) {
+ synchronized (topicCounterMap) {
Review comment:
Could it be replaced by ```computeIfPresent```?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#issuecomment-626188379
> I was unable to modify the existing test methods. This RoundRobinPartitioner#onNewBatch is not being used anywhere in the existing test methods, hence there was no scope to check.
The onNewBatch overridden method only decrementing the counter in the map, so it needs to be tested from a method where it is being called.
For example, you can verify the output of ```RoundRobinPartitioner#partition``` after calling ```RoundRobinPartitioner#onNewBatch``` to make sure the output is what you expect.
```java
Partitioner partitioner = new RoundRobinPartitioner();
Cluster cluster = new Cluster("clusterId", asList(NODES[0], NODES[1], NODES[2]), partitions,
Collections.<String>emptySet(), Collections.<String>emptySet());
int part = partitioner.partition("test", null, null, null, null, cluster);
assertEqual(expected, part);
partitioner.onNewBatch ("test", cluster, part);
part = partitioner.partition("test", null, null, null, null, cluster);
assertEqual(expected, part);
```
(this code is sample)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] AshishRoyJava commented on pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
AshishRoyJava commented on pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#issuecomment-626194182
@chia7712 Thanks for helping me out. I am new here and trying to contribute for the first time.
It will take me some time to go understand the workflow completely.
However, I have added a new test case and looks fine to me. Please review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #8633:
URL: https://github.com/apache/kafka/pull/8633#discussion_r422505119
##########
File path: clients/src/main/java/org/apache/kafka/clients/producer/RoundRobinPartitioner.java
##########
@@ -73,4 +73,12 @@ private int nextValue(String topic) {
public void close() {}
+ @Override
+ public void onNewBatch(String topic, Cluster cluster, int prevPartition) {
+ synchronized (topicCounterMap) {
Review comment:
the ```synchronized``` block is redundant, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] AshishRoyJava closed pull request #8633: KAFKA-9965: Incrementing counter cause uneven distribution with RoundRobinPartitioner
Posted by GitBox <gi...@apache.org>.
AshishRoyJava closed pull request #8633:
URL: https://github.com/apache/kafka/pull/8633
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org