helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet statistics ...................................................................... Patch Set 13: (12 comments) http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/integration-tests/tablet_replacement-itest.cc File src/kudu/integration-tests/tablet_replacement-itest.cc: http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/integration-tests/tablet_replacement-itest.cc@160 PS13, Line 160: Substitute("--update_tablet_stats_interval_ms=$0", (kTsToMasterHbIntervalSec + 1) * 1000), > Could you add a comment explaining why this is necessary? Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/catalog_manager.h@219 PS13, Line 219: // Reported tablet statistics (in-memory only), replica_id -> ReportedTabletStatsPB. : st > nit: strange spacing? Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/catalog_manager.h@325 PS13, Line 325: MasterTableStats* metrics() { return metrics_.get(); } > This is beginning to conflate "metrics" and "stats". To avoid confusion dow Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/catalog_manager.h@1042 PS13, Line 1042: // Just for tests. > nit: may be worth making note of a small example of why this would be usefu Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/catalog_manager.cc@5453 PS13, Line 5453: nullptr == metrics_ > nit: could just be Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/master-test.cc@1782 PS13, Line 1782: TEST_F(MasterTest, TestTableStats) { > A lot of this test seems to be setup to emulate a real cluster. Instead of Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/master/master-test.cc@1892 PS13, Line 1892: // Shutdown the background task. > Why was this necessary? Because there are not real tservers, the background task will disturb the tablet state. http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tablet/metadata.proto File src/kudu/tablet/metadata.proto: http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tablet/metadata.proto@193 PS13, Line 193: // A mirror of struct TabletMetrics, right now only 2 fields. > nit: How about something more generic, like: Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tablet/tablet_replica.h@310 PS13, Line 310: ReportedTabletStatsPB > How about returning a const ReportedTabletStatsPB&? Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tablet/tablet_replica.h@310 PS13, Line 310: TabletStatsPB > nit: for functions that just return an internal member, we usually just nam Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tablet/tablet_replica.h@388 PS13, Line 388: // Tablet stats object which is used for heartbeat. > nit: I don't think we need to tie heartbeating into this description. How a Done http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tserver/heartbeater.h File src/kudu/tserver/heartbeater.h: http://gerrit.cloudera.org:8080/#/c/13426/13/src/kudu/tserver/heartbeater.h@55 PS13, Line 55: it is > nit: they are Done -- To view, visit http://gerrit.cloudera.org:8080/13426 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74406ab7cca7c22fda455c328b8ee9989a6b2d99 Gerrit-Change-Number: 13426 Gerrit-PatchSet: 13 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Fri, 21 Jun 2019 09:24:03 +0000 Gerrit-HasComments: Yes
