wuchong commented on code in PR #1446:
URL: https://github.com/apache/fluss/pull/1446#discussion_r2252969237
##########
fluss-server/src/test/java/com/alibaba/fluss/server/zk/ZooKeeperClientTest.java:
##########
@@ -220,6 +220,31 @@ void testBatchCreateLeaderAndIsr() throws Exception {
}
}
+ @Test
+ void testBatchUpdateLeaderAndIsr() throws Exception {
+ int totalCount = 100;
+
+ // try to register bucket leaderAndIsr
+ Map<TableBucket, LeaderAndIsr> leaderAndIsrList = new HashMap<>();
+ for (int i = 0; i < totalCount; i++) {
+ TableBucket tableBucket = new TableBucket(1, i);
+ LeaderAndIsr leaderAndIsr =
+ new LeaderAndIsr(i, 10, Arrays.asList(i + 1, i + 2, i +
3), 100, 1000);
+ leaderAndIsrList.put(tableBucket, leaderAndIsr);
+ zookeeperClient.registerLeaderAndIsr(tableBucket, leaderAndIsr);
+ }
Review Comment:
Is it necessary to call `register leaderAndIsr`? I'm a bit confused about
this code because this makes even if `batchUpdateLeaderAndIsr` fails, the
subsequent assertion on `leaderAndIsr` could still pass. For example, when I
remove line 238 (`zookeeperClient.batchUpdateLeaderAndIsr(leaderAndIsrList)`),
the test still passes. Maybe we need to remove this part code.
##########
fluss-server/src/test/java/com/alibaba/fluss/server/coordinator/CoordinatorEventProcessorTest.java:
##########
@@ -761,6 +766,59 @@ void testNotifyOffsetsWithShrinkISR(@TempDir Path tempDir)
throws Exception {
verifyReceiveRequestExceptFor(3, leader,
NotifyKvSnapshotOffsetRequest.class);
}
+ @Test
+ void testProcessAdjustIsr() throws Exception {
Review Comment:
This test seems doing a void testing. It gets the current `LeaderAndIsr`s
and update `LeaderAndIsr`s without any changes, and then verify the
`LeaderAndIsr` again. If the `processAdjustIsr` has something wrong, this test
can't find anything.
I suggest to do some modification on the retrieved `LeaderAndIsr` from ctx,
like changing the leader to the second element in isr, and then send the new
`LeaderAndIsr` within `AdjustIsrReceivedEvent` to coordinator, finally,
asserting the `LeaderAndIsr` in ctx and response is updated to the new leader.
--
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]
