Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24945 )
Change subject: KUDU-3802 add cluster_leader_skew gauge ...................................................................... Patch Set 2: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/integration-tests/registration-test.cc File src/kudu/integration-tests/registration-test.cc: http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/integration-tests/registration-test.cc@306 PS2, Line 306: TEST_F(RegistrationTest, TestTSReportsRaftLeaderCount) { nit: consider checking an invariant of the number of alive replicas vs the number of Raft leaders per tablet server -- the former must always be greater or equal than the latter, and in case of RF=1 they must be equal once all the Raft leaders are established http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/integration-tests/registration-test.cc@316 PS2, Line 316: const auto num_raft_leaders nit for here and below: avoid copying std::optional and use const reference? http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/master/master-test.cc@904 PS2, Line 904: FLAGS_tserver_unresponsive_timeout_ms = 500; : ASSERT_EVENTUALLY([&] { : NO_FATALS(heartbeat("ts-1", /*reg=*/false, 1)); : NO_FATALS(heartbeat("ts-3", /*reg=*/false, 3)); : ASSERT_EQ(2, skew()); : }); Does this approach have flakiness potential? IIUC, the way how ASSERT_EVENTUALLY retries the block could lead to gaps more than 500ms in between, so if few initial fast retries didn't end up as expected with the timing (e.g., due to scheduler anomalies or overloaded testing node), eventually all the tablet servers would be considered unresponsive. http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/master/master_service.cc@466 PS2, Line 466: req->num_raft_leaders() nit: since it's a signed type, consider adding a DCHECK_GE(..., 0) on this for catching unexpected behavior and bugs -- I couldn't see a validation for this from the point where the value set on the tserver's metric up to the point it's being sent out by a tserver's Heartbeater http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/master/ts_manager.cc File src/kudu/master/ts_manager.cc: http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/master/ts_manager.cc@380 PS2, Line 380: const TSDescriptorMap::value_type& entry style nit: consider using structured bindings here for (const auto& [_, ts] : servers_by_id_) { ... } -- To view, visit http://gerrit.cloudera.org:8080/24945 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec82f841934f4e310509ee1b7156040b3bba5489 Gerrit-Change-Number: 24945 Gerrit-PatchSet: 2 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Tue, 29 Sep 2026 17:02:24 +0000 Gerrit-HasComments: Yes
