jsancio commented on code in PR #16079:
URL: https://github.com/apache/kafka/pull/16079#discussion_r1627947377
##########
raft/src/test/java/org/apache/kafka/raft/internals/KafkaRaftMetricsTest.java:
##########
@@ -98,6 +100,7 @@ private VoterSet localStandaloneVoterSet(short kraftVersion)
{
@ValueSource(shorts = {0, 1})
public void shouldRecordVoterQuorumState(short kraftVersion) {
boolean withDirectoryId = kraftVersion > 0;
+ Set<Integer> voterSet = Utils.mkSet(localId, 1, 2);
Review Comment:
Let's remove this line and instead use `voters.voterIds()` where needed.
##########
raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java:
##########
@@ -272,20 +280,109 @@ public void testUpdateHighWatermarkQuorumSizeThree() {
assertEquals(Optional.empty(), state.highWatermark());
assertTrue(state.updateReplicaState(node2, 0, new
LogOffsetMetadata(15L)));
assertEquals(Optional.of(new LogOffsetMetadata(15L)),
state.highWatermark());
- assertFalse(state.updateLocalState(new LogOffsetMetadata(20L)));
+ assertFalse(state.updateLocalState(new LogOffsetMetadata(20L),
voterSet));
assertEquals(Optional.of(new LogOffsetMetadata(15L)),
state.highWatermark());
assertTrue(state.updateReplicaState(node1, 0, new
LogOffsetMetadata(20L)));
assertEquals(Optional.of(new LogOffsetMetadata(20L)),
state.highWatermark());
assertFalse(state.updateReplicaState(node2, 0, new
LogOffsetMetadata(20L)));
assertEquals(Optional.of(new LogOffsetMetadata(20L)),
state.highWatermark());
}
+ @Test
+ public void testUpdateHighWatermarkAddingFollowerToVoterStates() {
Review Comment:
Can we add a test for the decreasing case? For example this change should
not decrease the HWM.
```text
voter 1 LEO: 15
voter 2 LEO: 15
voter 3 LEO: 10
```
The HWM is 15.
Adding voter 4 should not decrease the HWM to 10
```text
voter 1 LEO: 15
voter 2 LEO: 15
voter 3 LEO: 10
voter 4 LEO: 5
```
The HWM should be 15 even though the naive computation would return 10.
##########
raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java:
##########
@@ -272,20 +280,109 @@ public void testUpdateHighWatermarkQuorumSizeThree() {
assertEquals(Optional.empty(), state.highWatermark());
assertTrue(state.updateReplicaState(node2, 0, new
LogOffsetMetadata(15L)));
assertEquals(Optional.of(new LogOffsetMetadata(15L)),
state.highWatermark());
- assertFalse(state.updateLocalState(new LogOffsetMetadata(20L)));
+ assertFalse(state.updateLocalState(new LogOffsetMetadata(20L),
voterSet));
assertEquals(Optional.of(new LogOffsetMetadata(15L)),
state.highWatermark());
assertTrue(state.updateReplicaState(node1, 0, new
LogOffsetMetadata(20L)));
assertEquals(Optional.of(new LogOffsetMetadata(20L)),
state.highWatermark());
assertFalse(state.updateReplicaState(node2, 0, new
LogOffsetMetadata(20L)));
assertEquals(Optional.of(new LogOffsetMetadata(20L)),
state.highWatermark());
}
+ @Test
+ public void testUpdateHighWatermarkAddingFollowerToVoterStates() {
+ int node1 = 1;
+ int node2 = 2;
+ Set<Integer> originalVoterSet = mkSet(localId, node1);
+ LeaderState<?> state = newLeaderState(originalVoterSet, 10L);
+ assertFalse(state.updateLocalState(new LogOffsetMetadata(15L),
originalVoterSet));
+ assertFalse(state.updateReplicaState(node1, 0, new
LogOffsetMetadata(10L)));
+ assertEquals(Optional.empty(), state.highWatermark());
Review Comment:
The HWM should be 10, no?
##########
raft/src/test/java/org/apache/kafka/raft/LeaderStateTest.java:
##########
@@ -272,20 +280,109 @@ public void testUpdateHighWatermarkQuorumSizeThree() {
assertEquals(Optional.empty(), state.highWatermark());
assertTrue(state.updateReplicaState(node2, 0, new
LogOffsetMetadata(15L)));
assertEquals(Optional.of(new LogOffsetMetadata(15L)),
state.highWatermark());
- assertFalse(state.updateLocalState(new LogOffsetMetadata(20L)));
+ assertFalse(state.updateLocalState(new LogOffsetMetadata(20L),
voterSet));
assertEquals(Optional.of(new LogOffsetMetadata(15L)),
state.highWatermark());
assertTrue(state.updateReplicaState(node1, 0, new
LogOffsetMetadata(20L)));
assertEquals(Optional.of(new LogOffsetMetadata(20L)),
state.highWatermark());
assertFalse(state.updateReplicaState(node2, 0, new
LogOffsetMetadata(20L)));
assertEquals(Optional.of(new LogOffsetMetadata(20L)),
state.highWatermark());
}
+ @Test
+ public void testUpdateHighWatermarkAddingFollowerToVoterStates() {
+ int node1 = 1;
+ int node2 = 2;
+ Set<Integer> originalVoterSet = mkSet(localId, node1);
+ LeaderState<?> state = newLeaderState(originalVoterSet, 10L);
+ assertFalse(state.updateLocalState(new LogOffsetMetadata(15L),
originalVoterSet));
+ assertFalse(state.updateReplicaState(node1, 0, new
LogOffsetMetadata(10L)));
+ assertEquals(Optional.empty(), state.highWatermark());
+
+ // updating replica state of 2 before it joins voterSet should not
increase HW to 15L
+ assertFalse(state.updateReplicaState(node2, 0, new
LogOffsetMetadata(15L)));
+ assertEquals(Optional.empty(), state.highWatermark());
Review Comment:
The HWM should be 10, no?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
