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

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

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



##########
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:
       We have three separate argument checks to perform, each with its separate error message that the test check for:
   
   a) replication factory can't be non-positive
   b) unfenced broker count can't be 0
   c) unfenced broker count must must not be less than replication factor
   
   `StripedReplicaPlacer.place()` was performing checks (b) and (c).
   `RackList.place()` was performing check (a)
   
   Given that `RackList` is publicly accessible within the package, I felt it was important to perform all the sanity checks there. But `StripedReplicaPlacer.place()` also has a loop and allocates an array, so while we could allow the first iteration of the loop to raise the exception, I felt it was clearer to perform the checks there as well.
   




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