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/07/29 07:22:25 UTC
[GitHub] [kafka] showuon opened a new pull request #9096: Add comments to constrainedAssign and generalAssign method
showuon opened a new pull request #9096:
URL: https://github.com/apache/kafka/pull/9096
Recently, I tried to read the codes to understand what the `constrainedAssign` and `generalAssign` method is doing, but it is so difficult and suffering due to the complexity of the algorithm. And then I traced back to the JIRA ticket and KIP to get much more information about them. So, I think we should put the algorithm goal and main steps in the code comments, to let other developers better understand them and better do trouble shooting if any.
**reference for `constrainedAssign` algorithm:**
https://issues.apache.org/jira/browse/KAFKA-9987?focusedCommentId=17106832&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17106832
**reference for `generalAssign` algorithm:**
https://cwiki.apache.org/confluence/display/KAFKA/KIP-54+-+Sticky+Partition+Assignment+Strategy
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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] abbccdda commented on pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-668181513
Only flaky test failures.
----------------------------------------------------------------
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] abbccdda commented on pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-668113921
retest this please
----------------------------------------------------------------
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] showuon commented on pull request #9096: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-665472409
@ableegoldman @vahidhashemian , please help review this PR to add comments to the assign methods. Thanks.
----------------------------------------------------------------
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] showuon commented on a change in pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#discussion_r463371979
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractStickyAssignor.java
##########
@@ -163,6 +183,7 @@ private boolean allSubscriptionsEqual(Set<String> allTopics,
Map<String, List<TopicPartition>> assignment = new HashMap<>(
consumerToOwnedPartitions.keySet().stream().collect(Collectors.toMap(c -> c, c -> new ArrayList<>(minQuota))));
+ // step 1: Reassign as many previously owned partitions as possible
Review comment:
Updated. Thanks @abbccdda .
----------------------------------------------------------------
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] ableegoldman commented on a change in pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#discussion_r463752139
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractStickyAssignor.java
##########
@@ -144,15 +144,35 @@ private boolean allSubscriptionsEqual(Set<String> allTopics,
return true;
}
+
+ /**
+ * This constrainedAssign optimizes the assignment algorithm when all consumers were subscribed to same set of topics.
+ * The method includes the following steps:
+ *
+ * 1. Reassign as many previously owned partitions as possible
Review comment:
```suggestion
* 1. Reassign as many previously owned partitions as possible, up to the maxQuota
```
----------------------------------------------------------------
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] abbccdda merged pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
abbccdda merged pull request #9096:
URL: https://github.com/apache/kafka/pull/9096
----------------------------------------------------------------
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] abbccdda commented on pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-667819088
retest this please
----------------------------------------------------------------
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] abbccdda commented on pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-667173698
retest this please
----------------------------------------------------------------
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 #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-665504255
retest this please
----------------------------------------------------------------
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] showuon commented on pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-667784086
Thanks @ableegoldman . I've updated based on your suggestion. Thanks.
----------------------------------------------------------------
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] abbccdda commented on a change in pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
abbccdda commented on a change in pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#discussion_r463260878
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractStickyAssignor.java
##########
@@ -163,6 +183,7 @@ private boolean allSubscriptionsEqual(Set<String> allTopics,
Map<String, List<TopicPartition>> assignment = new HashMap<>(
consumerToOwnedPartitions.keySet().stream().collect(Collectors.toMap(c -> c, c -> new ArrayList<>(minQuota))));
+ // step 1: Reassign as many previously owned partitions as possible
Review comment:
nit: I don't think we need steps here, which makes it hard to squeeze in more comments later.
----------------------------------------------------------------
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] abbccdda commented on pull request #9096: MINOR: Add comments to constrainedAssign and generalAssign method
Posted by GitBox <gi...@apache.org>.
abbccdda commented on pull request #9096:
URL: https://github.com/apache/kafka/pull/9096#issuecomment-666894017
retest this please
----------------------------------------------------------------
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