cmccabe commented on code in PR #15876:
URL: https://github.com/apache/kafka/pull/15876#discussion_r1599222750
##########
metadata/src/main/java/org/apache/kafka/controller/PartitionChangeBuilder.java:
##########
@@ -235,10 +245,16 @@ ElectionResult electLeader() {
/**
* Assumes that the election type is Election.PREFERRED
*/
- private ElectionResult electPreferredLeader() {
- int preferredReplica = targetReplicas.get(0);
- if (isValidNewLeader(preferredReplica)) {
- return new ElectionResult(preferredReplica, false);
+ private ElectionResult electPreferredOrDesignatedLeader() {
+ if (election == Election.PREFERRED) {
+ int preferredReplica = targetReplicas.get(0);
+ if (isValidNewLeader(preferredReplica)) {
+ return new ElectionResult(preferredReplica, false);
+ }
+ } else if (election == Election.DESIGNTATED) {
+ if (isValidNewLeader(desiredLeader)) {
+ return new ElectionResult(desiredLeader, false);
+ }
Review Comment:
If the designated leader isn't valid, shouldn't we return an error code
rather than falling through? I don't think we want to just elect "whatever" in
the case where we couldn't do what the user asked us to do.
On a related note, I don't see the point of having a single function for
both elect preferred and elect designated. It seems like we should have
separate functions for each?
--
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
