[GitHub] [kafka] junrao commented on a change in pull request #10823: KAFKA-12897: KRaft multi-partition placement on single broker

[GitBox <gi...@apache.org>]

junrao commented on a change in pull request #10823:
URL: https://github.com/apache/kafka/pull/10823#discussion_r645905158



##########
File path: metadata/src/main/java/org/apache/kafka/controller/StripedReplicaPlacer.java
##########
@@ -412,14 +433,9 @@ public StripedReplicaPlacer(Random random) {
                                      short replicationFactor,
                                      Iterator<UsableBroker> iterator) {
         RackList rackList = new RackList(random, iterator);
-        if (rackList.numUnfencedBrokers() == 0) {
-            throw new InvalidReplicationFactorException("All brokers are currently fenced.");
-        }
-        if (replicationFactor > rackList.numTotalBrokers()) {
-            throw new InvalidReplicationFactorException("The target replication factor " +
-                "of " + replicationFactor + " cannot be reached because only " +
-                rackList.numTotalBrokers() + " broker(s) are registered.");
-        }
+        throwInvalidReplicationFactorIfNonPositive(replicationFactor);
+        throwInvalidReplicationFactorIfZero(rackList.numUnfencedBrokers());
+        throwInvalidReplicationFactorIfTooFewBrokers(replicationFactor, rackList.numTotalBrokers());

Review comment:
       It seems that we are doing the same tests in rackList.place(). Should we just do the tests in one place?

##########
File path: metadata/src/main/java/org/apache/kafka/controller/StripedReplicaPlacer.java
##########
@@ -340,14 +340,14 @@ int numUnfencedBrokers() {
         }
 
         List<Integer> place(int replicationFactor) {
-            if (replicationFactor <= 0) {
-                throw new InvalidReplicationFactorException("Invalid replication factor " +
-                        replicationFactor + ": the replication factor must be positive.");
-            }
+            throwInvalidReplicationFactorIfNonPositive(replicationFactor);
+            throwInvalidReplicationFactorIfTooFewBrokers(replicationFactor, numTotalBrokers());
+            throwInvalidReplicationFactorIfZero(numUnfencedBrokers());
             // If we have returned as many assignments as there are unfenced brokers in
             // the cluster, shuffle the rack list and broker lists to try to avoid
             // repeating the same assignments again.
-            if (epoch == numUnfencedBrokers) {
+            // But don't reset the iteration epoch for a single unfenced broker -- otherwise we would loop forever
+            if (epoch == numUnfencedBrokers && numUnfencedBrokers > 1) {

Review comment:
       Which loop loops forever because of this? Also, is there an existing test covering this? 




-- 
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