Adar Dembo has posted comments on this change. Change subject: Add tablet state summary metrics ......................................................................
Patch Set 1: (5 comments) Has anything materially changed since the split? http://gerrit.cloudera.org:8080/#/c/7980/1/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: PS1, Line 79: METRIC_DECLARE_gauge_int32(tablets_num_running); : METRIC_DECLARE_gauge_int32(tablets_num_bootstrapping); Nit: move this down to where the other metrics are declared? PS1, Line 1545: SleepFor(MonoDelta::FromMilliseconds(15)); : ASSERT_EQ(1, CountRunningTablets(cluster_->tablet_server(follower_index))); Can you use ASSERT_EVENTUALLY rather than sleeping? Below too. http://gerrit.cloudera.org:8080/#/c/7980/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS1, Line 1259: // pass : break; Got some tabs in here. PS1, Line 1267: std::lock_guard<rw_spinlock> lock(lock_); : UpdateTabletStateSummary(); : return tablet_state_summary_.num_not_initialized; Since this pattern is repeated a lot, perhaps you could change UpdateTabletStateSummary to accommodate it: 1. Have it acquire/release the lock, and 2. Have it return the appropriate counter value (specified via argument). http://gerrit.cloudera.org:8080/#/c/7980/1/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: Line 308: void UpdateTabletStateSummary(); Nit: convention is to add the "Unlocked" suffix to functions that must be called with a lock held. -- To view, visit http://gerrit.cloudera.org:8080/7980 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
