Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21580 )
Change subject: [client] add ScanTokenStaleRaftMembershipTest ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/21580/1/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/21580/1/src/kudu/client/scan_token-test.cc@1590 PS1, Line 1590: // ??? > nit: Remove Done -- removed the customization for the flag altogether since it's not crucial for the essence of the new test. http://gerrit.cloudera.org:8080/#/c/21580/1/src/kudu/client/scan_token-test.cc@1702 PS1, Line 1702: ASSERT_OK(LeaderStepDown(tsd, tablet_id, MonoDelta::FromSeconds(30))); > Do you think it would makes sense to add an assert to verify the new_leader It's a good point. Even if that's covered by existing tests scenarios for Raft consensus implementation, I think it makes sense to add an extra check here since that's crucial for the essence of this scenario. http://gerrit.cloudera.org:8080/#/c/21580/1/src/kudu/client/scan_token-test.cc@1721 PS1, Line 1721: ASSERT_LT(init_location_requests, NumGetTableLocationsRequests()) > nit: Can we do a more tighter ASSERT_EQ on init_location_requests + 1 and Why? The actual number of GetTableLocation requests performed is hard to predict, and it might be significantly higher than init_location_requests. It all depends on how long Raft election takes to elect a new leader replica and how fast the client retries its write RPC. I updated the comments to state it more clearly that the request to transfer the leadership just makes the current leader to step down, but it doesn't wait for a new leader to be elected. -- To view, visit http://gerrit.cloudera.org:8080/21580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ce3d549d4ab2502c58deae1250b49ba16bbc914 Gerrit-Change-Number: 21580 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sat, 13 Jul 2024 00:40:00 +0000 Gerrit-HasComments: Yes
