jsancio commented on code in PR #16563:
URL: https://github.com/apache/kafka/pull/16563#discussion_r1674382183


##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -281,15 +281,20 @@ public void testGrantVotesWhenShuttingDown(boolean 
withKip853Rpc) throws Excepti
         context.client.poll();
 
         // We will first transition to unattached and then grant vote and then 
transition to voted
-        assertTrue(context.client.quorum().isVoted());
+        assertTrue(
+            context.client.quorum().isVoted(),
+            "Local Id: " + localId +
+                " Remote Id: " + remoteId +
+                " Quorum local Id: " + 
context.client.quorum().localIdOrSentinel()
+                + " Quorum leader Id: " + 
context.client.quorum().leaderIdOrSentinel());

Review Comment:
   How about:
   ```java
               "Local Id: " + localId +
               " Remote Id: " + remoteId +
               " Quorum local Id: " + 
context.client.quorum().localIdOrSentinel() +
               " Quorum leader Id: " + 
context.client.quorum().leaderIdOrSentinel()
           );
   ```



##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -1977,12 +1978,20 @@ private static ReplicaKey replicaKey(int id, boolean 
withDirectoryId) {
         return ReplicaKey.of(id, directoryId);
     }
 
+    private static Integer randomReplicaId() {
+        int mockAddressPrefix = 9990;
+        // Number of nodes we can set up if we got a random number that is 
maximum in the range
+        int reservedNumberOfPorts = 50;
+        int maxPort = 65535 - mockAddressPrefix - reservedNumberOfPorts;
+        return ThreadLocalRandom.current().nextInt(maxPort);
+    }

Review Comment:
   Let's return an `int` instead of the object `Integer`: `private static int 
randomReplicaId()`.
   
   The variable names `mockAddressPrefix` and `reserverdNumberOfPorts` are 
misleading this id has nothing to do with those concepts. If you want to limit 
the range of the integer, I am okay with generating a random id from 0 to 1024.



##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientTest.java:
##########
@@ -3860,12 +3865,11 @@ private static ReplicaKey replicaKey(int id, boolean 
withDirectoryId) {
         return ReplicaKey.of(id, directoryId);
     }
 
-    private static Integer getRandomPort() {
-        int minPort = 1024;
+    private static Integer randomReplicaId() {
         int mockAddressPrefix = 9990;
         // Number of nodes we can set up if we got a random number that is 
maximum in the range
         int reservedNumberOfPorts = 50;
         int maxPort = 65535 - mockAddressPrefix - reservedNumberOfPorts;

Review Comment:
   Let's return an `int` instead of the object `Integer`: `private static int 
randomReplicaId()`.
   
   The variable names `mockAddressPrefix` and `reserverdNumberOfPorts` are 
misleading this id has nothing to do with those concepts. If you want to limit 
the range of the integer, I am okay with generating a random id from 0 to 1024.



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

Reply via email to