Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/24945 )
Change subject: KUDU-3802 add cluster_leader_skew gauge ...................................................................... Patch Set 3: (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 Done http://gerrit.cloudera.org:8080/#/c/24945/2/src/kudu/integration-tests/registration-test.cc@316 PS2, Line 316: const auto& num_raft_leader > nit for here and below: avoid copying std::optional and use const reference Done 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: // : // The timeout has to be long enough that a pause between the heartbeats : // forged below and the gauge read that follows them can't make a server that : // just heartbeated look dead -- TSAN and ASAN builds stall for a while -- and : // short enough that waiting 'ts-0' out fits in ASSERT_EVENTUALLY's budget. : // > Ah, probably this isn't as bad as I thought initially: calling skew() immed Right, the skew() read does happen immediately after the heartbeats. I've raised the timeout to 2000 ms anyway: 500 ms was the tightest value in the tree (auto_rebalancer-test.cc uses 1000 ms and 5000 ms), and widening it only costs a couple of extra retries while 'ts-0' ages out, well inside ASSERT_EVENTUALLY's 30 second budget. I ran the scenario 5 times in a DEBUG build and it was green, though I haven't tried it under TSAN or ASAN. 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 I think the check is already there, one call deeper: TSDescriptor::set_num_raft_leaders() opens with DCHECK_GE(n, 0) in ts_descriptor.h, which is where set_num_live_replicas() just above it validates num_live_tablets() from the same heartbeat. Happy to add one here as well if you'd rather have it visible at the call site, but as it stands I think it would be a duplicate. 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: d_lock l(lock_); > style nit: consider using structured bindings here Done -- 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: 3 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: Thu, 01 Oct 2026 20:27:05 +0000 Gerrit-HasComments: Yes
