Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
jolshan merged PR #15359: URL: https://github.com/apache/kafka/pull/15359 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
jolshan commented on PR #15359: URL: https://github.com/apache/kafka/pull/15359#issuecomment-1945185518 I will wait a day or to see if @cmccabe has any comments. If not I will merge. 👍 -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
CalvinConfluent commented on PR #15359: URL: https://github.com/apache/kafka/pull/15359#issuecomment-1944225912 testIOExceptionDuringCheckpoint failure is irrelevant to the PR. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
jolshan commented on code in PR #15359:
URL: https://github.com/apache/kafka/pull/15359#discussion_r1488779741
##
metadata/src/main/java/org/apache/kafka/controller/PartitionReassignmentReplicas.java:
##
@@ -120,9 +120,7 @@ Optional
maybeCompleteReassignment(List targetIs
}
if (newTargetReplicas.isEmpty()) return Optional.empty();
}
-for (int replica : adding) {
-if (!newTargetIsr.contains(replica)) return Optional.empty();
Review Comment:
Hmm this code is very confusing. Thanks for clarifying.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
CalvinConfluent commented on code in PR #15359:
URL: https://github.com/apache/kafka/pull/15359#discussion_r1488779105
##
metadata/src/main/java/org/apache/kafka/controller/PartitionReassignmentReplicas.java:
##
@@ -120,9 +120,7 @@ Optional
maybeCompleteReassignment(List targetIs
}
if (newTargetReplicas.isEmpty()) return Optional.empty();
}
-for (int replica : adding) {
-if (!newTargetIsr.contains(replica)) return Optional.empty();
Review Comment:
Yes, the replica is the original replica + adding replica. Double-checked in
UT debug mode.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
jolshan commented on code in PR #15359:
URL: https://github.com/apache/kafka/pull/15359#discussion_r1488762961
##
metadata/src/main/java/org/apache/kafka/controller/PartitionReassignmentReplicas.java:
##
@@ -120,9 +120,7 @@ Optional
maybeCompleteReassignment(List targetIs
}
if (newTargetReplicas.isEmpty()) return Optional.empty();
}
-for (int replica : adding) {
-if (!newTargetIsr.contains(replica)) return Optional.empty();
Review Comment:
Are you sure?
```
newTargetReplicas = new ArrayList<>(replicas.size());
for (int replica : replicas) {
if (!removing.contains(replica)) {
newTargetReplicas.add(replica);
}
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
jolshan commented on code in PR #15359:
URL: https://github.com/apache/kafka/pull/15359#discussion_r1488762961
##
metadata/src/main/java/org/apache/kafka/controller/PartitionReassignmentReplicas.java:
##
@@ -120,9 +120,7 @@ Optional
maybeCompleteReassignment(List targetIs
}
if (newTargetReplicas.isEmpty()) return Optional.empty();
}
-for (int replica : adding) {
-if (!newTargetIsr.contains(replica)) return Optional.empty();
Review Comment:
Are you sure?
```
for (int replica : replicas) {
if (!removing.contains(replica)) {
newTargetReplicas.add(replica);
}
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
CalvinConfluent commented on code in PR #15359:
URL: https://github.com/apache/kafka/pull/15359#discussion_r1488746986
##
metadata/src/main/java/org/apache/kafka/controller/PartitionReassignmentReplicas.java:
##
@@ -120,9 +120,7 @@ Optional
maybeCompleteReassignment(List targetIs
}
if (newTargetReplicas.isEmpty()) return Optional.empty();
}
-for (int replica : adding) {
-if (!newTargetIsr.contains(replica)) return Optional.empty();
Review Comment:
The newTargetReplicas is the original replica + adding - removing.
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
jolshan commented on code in PR #15359:
URL: https://github.com/apache/kafka/pull/15359#discussion_r1488713079
##
metadata/src/main/java/org/apache/kafka/controller/PartitionReassignmentReplicas.java:
##
@@ -120,9 +120,7 @@ Optional
maybeCompleteReassignment(List targetIs
}
if (newTargetReplicas.isEmpty()) return Optional.empty();
}
-for (int replica : adding) {
-if (!newTargetIsr.contains(replica)) return Optional.empty();
Review Comment:
Do we still need this? If a replica we are adding is not yet in the replicas
list?
--
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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15665: Enforce partition reassignment should complete when all target replicas are in ISR [kafka]
CalvinConfluent commented on PR #15359: URL: https://github.com/apache/kafka/pull/15359#issuecomment-1939789669 @splett2 @jolshan @artemlivshits @cmccabe With the discussion the the previous PR, I changed the logic to enforce to include all the target replicas in the ISR. -- 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. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
