Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14976 )
Change subject: KUDU-3011 p1: metric to count tablet leaders ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/consensus/raft_consensus.h File src/kudu/consensus/raft_consensus.h: http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/consensus/raft_consensus.h@85 PS3, Line 85: // Gauge indicating how many Raft leaders are hosted on the server. > Nit: "Raft tablet leaders" Done http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/consensus/raft_consensus.cc@701 PS3, Line 701: return AppendNewRoundToQueueUnlocked(round); > What if this fails? That's fine since we're already in leader mode; eventually another leader will be elected and this leader will step down. Also it's "fine" because this is wrapped by a CHECK_OK. http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/consensus/raft_consensus.cc@2907 PS3, Line 2907: if (server_ctx_.num_leaders) server_ctx_.num_leaders->IncrementBy(-1); > Do we also need to do this in RaftConsensus::Stop? Or something that's comm Yeah, good catch. Done. I moved this into BecomeReplicaUnlocked, but avoided the startup issue. http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/master/sys_catalog.cc@382 PS3, Line 382: RETURN_NOT_OK_SHUTDOWN(tablet_replica_->Init({ /*num_leaders*/nullptr, > Could be useful for masters though, right? To indicate which master is curr Yeah, though I'd prefer keeping this patch focused on the tserver leader count, unless you're suggesting moving the metric into kserver now (or some other shared class between tserver and master). http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/14976/3/src/kudu/tserver/ts_tablet_manager.h@410 PS3, Line 410: scoped_refptr<AtomicGauge<int32_t>> num_leaders_; > Maybe "num_tablet_leaders" to be more clear? Done -- To view, visit http://gerrit.cloudera.org:8080/14976 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa6554458a860e34f97af168da7ed786c8ef47e4 Gerrit-Change-Number: 14976 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sun, 05 Jan 2020 01:10:30 +0000 Gerrit-HasComments: Yes
