Mike Percy has posted comments on this change. Change subject: Add tablet state summary metrics ......................................................................
Patch Set 5: (4 comments) Looking pretty good, just a couple nits. http://gerrit.cloudera.org:8080/#/c/7980/5/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: Line 1530: workload.StopAndJoin(); Why not stop the workload on line 1523, as soon as we have the number of rows inserted that we want? The fact that we leave it running here made me think we were trying to test some concurrency condition here, but that's not the case. PS5, Line 1544: Sleep for > 10ms This comment seems out of date. http://gerrit.cloudera.org:8080/#/c/7980/5/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 115: DEFINE_int32(tablet_state_walk_min_period_ms, 1000, What do you think about putting the metrics prototypes, callbacks, and registration into its own class with its own file? For example, a TsTabletMangerMetrics class in a ts_tablet_manager_metrics.cc file like TabletMetrics is structured. That could keep the bulk of the boilerplate out of this file (which is already large), except perhaps for the gflag and TSTabletManager::UpdateTabletStateSummaryAndReturnCount() function, which probably can't be easily factored out. Line 1226: MonoDelta period = MonoDelta::FromMilliseconds(FLAGS_tablet_state_walk_min_period_ms); We can initialize this object before acquiring the lock -- 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: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
