Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet metrics ...................................................................... Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/master/catalog_manager.h@189 PS5, Line 189: ReportedTabletMetricsPB peer_metrics_return(); There may be multiple replica metrics associated with a tablet. Which gets returned? Is it an aggregate? A random one? Could use a comment. http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/master/catalog_manager.h@187 PS5, Line 187: // Upsert or delete the metrics. : void peer_metrics_upsert(const std::string& peer, const ReportedTabletMetricsPB& metrics); : ReportedTabletMetricsPB peer_metrics_return(); : void peer_metrics_delete(const std::string& peer); Looking at the other functions of the CatalogManager, perhaps these names would be more natural: void set_peer_metrics(const std::string& replica_id, const ReportedTabletMetricsPB& metrics); void unset_peer_metrics(const std::string& replica_id); ReportedTabletMetricsPB peer_metrics(); http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/master/catalog_manager.cc@5243 PS5, Line 5243: peer_metrics_upsert In newer code, we've generally used "replica" instead of "peer" when referring to TabletReplicas. Could you update the names to use "replica" instead of "peer" here and elsewhere? http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/master/master.proto@233 PS5, Line 233: MetricsPB I don't love the idea that this is called a "metric", since I think this could be extended in the future to be a more generalized statistic. How about calling these ReportedTabletStatsPB? http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.h File src/kudu/tserver/heartbeater.h: http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.h@76 PS5, Line 76: const MonoTime GetUpdateTime(); : void SetUpdateTimeForTests(const MonoTime& time); : void UpdateTabletMetrics(); : Status GetTabletMetricsPB(const std::string& tablet_id, master::ReportedTabletMetricsPB* pb); Could you add comments describing these and how a user of the Heartbeater class should use these? http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.cc@318 PS5, Line 318: // 1. nit: Could you remove these numbers? This function is short enough that it's relatively easy to follow the comments without them. http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.cc@344 PS5, Line 344: for (auto it = tablet_metrics.begin(); it != tablet_metrics.end(); ++it) { Should we remove metrics that don't exist in tablet_metrics (i.e. tablets that have been deleted)? Maybe rather than updating the old map, we can remove tablets that didn't change from the new map. http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.cc@343 PS5, Line 343: vector<string> dirty_tablets; : for (auto it = tablet_metrics.begin(); it != tablet_metrics.end(); ++it) { : auto iter = tablet_metrics_.find(it->first); : if (iter != tablet_metrics_.end() && : iter->second->disk_size() == it->second->disk_size() && : iter->second->row_count() == it->second->row_count()) { : continue; : } : : tablet_metrics_[it->first] = std::move(it->second); : dirty_tablets.emplace_back(it->first); : } It's hard to follow this because of how similar 'iter' and 'it' are. Maybe rewrite as something like: for (auto id_and_metrics : tablet_metrics) { const string& tablet_id = id_and_metrics.first; auto new_metrics = id_and_metrics.second; const auto* old_metrics = FindOrNull(tablet_metrics_, tablet_id); if (old_metrics && old_metrics->disk_size() == new_metrics->disk_size() && old_metrics->row_count() == new_metrics->row_count()) { continue; } EmplaceOrUpdate(&tablet_metrics_, tablet_id, std::move(metrics)); } http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.cc@357 PS5, Line 357: update_time_ = MonoTime::Now() + : MonoDelta::FromMilliseconds(FLAGS_update_tablet_metrics_interval_ms); nit: I think it'd be more intuitive to keep around a 'time_of_last_update_' or somesuch. 'update_time_' could be confused to mean "the time that we last updated the metrics" Also using 'time_of_last_update_' means we don't have to initialize its value to something meaningful I think, since it's never been updated. http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/heartbeater.cc@642 PS5, Line 642: // If that time expires, let's update the tablet metrics. : if (MonoTime::Now() > parent_->GetUpdateTime()) { : parent_->UpdateTabletMetrics(); : } How about encapsulating this check into the function itself, e.g. UpdateTabletMetricsIfNecessary() or something? Then we wouldn't have to expose the update time (or take the lock twice), no? On a related note, what do you think about putting this computation (including the checks for timing) into the TSTabletManager and the report-generating methods? If you did that, I don't think you'd have to update the Heartbeater at all (the heartbeating mechanism hasn't changed, after all). But rather, the TSTabletManager would track when to update and only add them to the report if appropriate. -- 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: 5 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: Tue, 11 Jun 2019 03:02:54 +0000 Gerrit-HasComments: Yes
