[GitHub] [kafka] jsancio commented on a change in pull request #10309: KAFKA-12181; Loosen raft fetch offset validation of remote replicas

[GitBox]

jsancio commented on a change in pull request #10309:
URL: https://github.com/apache/kafka/pull/10309#discussion_r593492361



##########
File path: raft/src/main/java/org/apache/kafka/raft/LeaderState.java
##########
@@ -247,7 +267,7 @@ private boolean isVoter(int remoteNodeId) {
         return voterReplicaStates.containsKey(remoteNodeId);
     }
 
-    private static class ReplicaState implements Comparable<ReplicaState> {
+    private static abstract class ReplicaState implements 
Comparable<ReplicaState> {

Review comment:
       Do we really need to distinguish between `VoterState` and 
`ObserverState`? For example, the only different is `hasAcknowledgedLeader`. I 
would argue that we could just move this field to `ReplicateState` and say that 
observers will have this value always false or the value is ignored.
   
   I am leaning towards just updating the value irrespective of if it is a 
voter or observer. This is probably useful to have it when we implement quorum 
reassignment. We can document whatever semantic you decide as a comment for 
this type.

##########
File path: raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java
##########
@@ -140,6 +160,22 @@ public void testUpdateHighWatermarkQuorumSizeThree() {
         assertEquals(Optional.of(new LogOffsetMetadata(20L)), 
state.highWatermark());
     }
 
+    @Test
+    public void testNonMonotonicHighWatermarkUpdate() {
+        MockTime time = new MockTime();
+        int node1 = 1;
+        LeaderState state = newLeaderState(mkSet(localId, node1), 0L);
+        state.updateLocalState(time.milliseconds(), new 
LogOffsetMetadata(10L));
+        state.updateReplicaState(node1, time.milliseconds(), new 
LogOffsetMetadata(10L));
+        assertEquals(Optional.of(new LogOffsetMetadata(10L)), 
state.highWatermark());
+
+        // Follower crashes and disk is lost. It fetches an earlier offset to 
rebuild state.
+        state.updateReplicaState(node1, time.milliseconds(), new 
LogOffsetMetadata(5L));

Review comment:
       Let's check that this calls returns `false`.
   
   Let's also add a test that calls `getVoterEndOffsets` and checks the 
returned map is correct.




----------------------------------------------------------------
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:
us...@infra.apache.org