kevin-wu24 commented on code in PR #21453:
URL: https://github.com/apache/kafka/pull/21453#discussion_r2931487944
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -2129,6 +2129,65 @@ public void
testCreateSnapshotAsFollowerWithInvalidSnapshotId(boolean withKip853
);
}
+ @Test
+ public void testListenerReceivesBootstrapSnapshot() throws Exception {
+ ReplicaKey localKey = replicaKey(0, true);
+ ReplicaKey otherNodeKey = replicaKey(localKey.id() + 1, true);
+ VoterSet voters = VoterSetTest.voterSet(Stream.of(localKey,
otherNodeKey));
+
+ RaftClientTestContext context = new RaftClientTestContext
+ .Builder(localKey.id(), localKey.directoryId().get())
+ .withKip853Rpc(true)
+ .withBootstrapSnapshot(Optional.of(voters))
+ .withUnknownLeader(3)
+ .build();
+
+ context.unattachedToLeader();
+ int epoch = context.currentEpoch();
+
+ long localLogEndOffset = context.log.endOffset().offset();
+ context.deliverRequest(context.fetchRequest(epoch, otherNodeKey,
localLogEndOffset, epoch, 0));
+ context.pollUntilResponse();
+ context.assertSentFetchPartitionResponse(Errors.NONE, epoch,
OptionalInt.of(localKey.id()));
+ assertEquals(localLogEndOffset,
context.client.highWatermark().getAsLong());
+
+ // Bootstrap snapshot should route to handleLoadBootstrap, not
handleLoadSnapshot
+ try (SnapshotReader<String> bootstrapSnapshot =
context.listener.drainHandledBootstrapSnapshot().get()) {
+ assertEquals(Snapshots.BOOTSTRAP_SNAPSHOT_ID,
bootstrapSnapshot.snapshotId());
+ }
+ assertFalse(context.listener.drainHandledSnapshot().isPresent());
+ }
+
+ @Test
+ public void testListenerReceivesCommittedSnapshotNotBootstrap() throws
Exception {
Review Comment:
I think this test is a bit redundant. We have other tests that check the
contents of non-bootstrap snapshots already. Maybe you can add your assert on
L2188 to some of them instead.
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -2129,6 +2129,65 @@ public void
testCreateSnapshotAsFollowerWithInvalidSnapshotId(boolean withKip853
);
}
+ @Test
+ public void testListenerReceivesBootstrapSnapshot() throws Exception {
+ ReplicaKey localKey = replicaKey(0, true);
+ ReplicaKey otherNodeKey = replicaKey(localKey.id() + 1, true);
+ VoterSet voters = VoterSetTest.voterSet(Stream.of(localKey,
otherNodeKey));
+
+ RaftClientTestContext context = new RaftClientTestContext
+ .Builder(localKey.id(), localKey.directoryId().get())
+ .withKip853Rpc(true)
+ .withBootstrapSnapshot(Optional.of(voters))
+ .withUnknownLeader(3)
+ .build();
+
+ context.unattachedToLeader();
+ int epoch = context.currentEpoch();
+
+ long localLogEndOffset = context.log.endOffset().offset();
+ context.deliverRequest(context.fetchRequest(epoch, otherNodeKey,
localLogEndOffset, epoch, 0));
Review Comment:
After reading through the code, it looks like the leader only updates
listener progress (and therefore calls `handleLoadBootstrap`) after
establishing a high watermark.
In the case of a voter set > 1, the leader needs to handle at least 1 fetch
request to establish a HWM, which is what you've done here.
In the case of a standalone leader, we need to append a batch in order to
establish a HWM. We should not need any incoming fetch requests in this case. I
think this case is better to test than what you have below in
`testListenerReceivesCommittedSnapshotNotBootstrap`.
--
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]
