Andrew Wong 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? 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? 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 down the line, I think we should make sure it is clear which one is which. In my opinion, they are different: metrics are displayed on the /metrics page for consumption by an operator or a plotting tool, whereas for stats, we may want to implement an API in Kudu to compute stats for querying engines like Impala to use (e.g. to get the data size, min, max, histogram, unique vals, etc., even exposing per-partition statistics vs aggregated statistics). It would have been nice to have merged the previous approach (that aggregated at the web UI), and then build metrics on top of that, as Mike suggested. It would have separated the concerns of stats, vs adding a new metric entity for tables. In any case, these methods here seem to only be concerned with displaying metrics, so I would consider renaming these functions/class to something like: void RegisterMetrics(const string& table_name, MetricRegistry* metric_registry); void UpdateMetrics(int on_disk_size_diff, int live_row_count_diff); TableMetrics* metrics(); 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 useful. 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 if (!metrics_) return; 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 doing this all manually, it may be easier to write and review if you add a test to integration-tests/ts_tablet_manager-itest.cc, which uses a mini-cluster and exercises heartbeats and tablet reporting. 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? 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: "Statistics for a tablet replica." This may not forever be limited to tablet metrics, e.g. we could store mins, maxes, histograms, etc. 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: TabletStatsPB nit: for functions that just return an internal member, we usually just name the function based on the internal member, e.g. stats_pb() 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&? 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 about: "Stats for the tablet replica." 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 -- 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 02:16:07 +0000 Gerrit-HasComments: Yes
