ahuang98 commented on code in PR #18600:
URL: https://github.com/apache/kafka/pull/18600#discussion_r1937607369
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -95,15 +103,15 @@ public void testLatestSnapshotIdMissing() throws Exception
{
}
@ParameterizedTest
- @CsvSource({ "false,false", "false,true", "true,false", "true,true" })
- public void testLeaderListenerNotified(boolean entireLog, boolean
withKip853Rpc) throws Exception {
+ @MethodSource("generateKip853TestMatrix")
+ public void testLeaderListenerNotified(boolean entireLog, RaftProtocol
raftProtocol) throws Exception {
Review Comment:
I guess another option would be just renaming the second parameter to
`boolean isReconfigSupported`, potentially easier for readers to know at a
glance that we're not testing the entire set of raftProtocols (rather than
navigating to the method source definition)
(then we would do `.withRaftProtocol(isReconfigSupported ? KIP_853_PROTOCOL
: KIP_595_PROTOCOL)` which I understand is a bit bulkier)
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -2265,4 +2267,17 @@ public ByteBuffer buffer() {
return data;
}
}
+
+ private boolean isSupport853(RaftProtocol raftProtocol) {
Review Comment:
you can use RaftClientTestContext.isReconfigSupported()
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -795,21 +800,20 @@ public void testFetchSnapshotRequestAsLeader(boolean
withKip853Rpc) throws Excep
}
@ParameterizedTest
- @ValueSource(booleans = { false, true })
- public void
testLeaderShouldResignLeadershipIfNotGetFetchSnapshotRequestFromMajorityVoters(
- boolean withKip853Rpc
- ) throws Exception {
+ @EnumSource(value = RaftProtocol.class, names = {KIP_595_PROTOCOL,
KIP_853_PROTOCOL})
+ public void
testLeaderShouldResignLeadershipIfNotGetFetchSnapshotRequestFromMajorityVoters(RaftProtocol
raftProtocol) throws Exception {
int localId = randomReplicaId();
- ReplicaKey voter1 = replicaKey(localId + 1, withKip853Rpc);
- ReplicaKey voter2 = replicaKey(localId + 2, withKip853Rpc);
- ReplicaKey observer3 = replicaKey(localId + 3, withKip853Rpc);
+ boolean support853 = isSupport853(raftProtocol);
Review Comment:
nit: `support853` -> `reconfigSupported`
##########
raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java:
##########
@@ -52,13 +54,19 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.Stream;
+import static org.apache.kafka.raft.RaftClientTestContext.Builder;
+import static org.apache.kafka.raft.RaftClientTestContext.MockListener;
+import static org.apache.kafka.raft.RaftClientTestContext.RaftProtocol;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
public final class KafkaRaftClientSnapshotTest {
+ private static final String KIP_595_PROTOCOL = "KIP_595_PROTOCOL";
+ private static final String KIP_853_PROTOCOL = "KIP_853_PROTOCOL";
Review Comment:
my slight pref would be to remove these and just use the strings directly in
the EnumSource annotations
--
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
