Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet statistics ...................................................................... Patch Set 9: (20 comments) http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/catalog_manager.h@187 PS9, Line 187: // Set tablet statistics. I'd like to see test coverage for replica_stats(). http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/catalog_manager.h@192 PS9, Line 192: ReportedTabletStatsPB replica_stats(const std::string& replica_id); This method should be const. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/catalog_manager.cc@5244 PS9, Line 5244: const ReportedTabletStatsPB& stats) { Can you pass this by value and std::move it into the map? http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/master.proto@234 PS9, Line 234: required int64 row_count = 1; Should doc that this is the 'live' row count, or that it excludes deleted rows. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/master/master_path_handlers.cc@394 PS9, Line 394: if (role == RaftPeerPB::LEADER) { Seems odd to do the filtering and aggregation in the web UI. Don't you also want to expose these values as master metrics, in which case all of this would have to be repeated elsewhere? Moreover, this doesn't adhere to the criteria that Mike outlined, wherein we handle the case where none of the replicas are LEADER yet. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/heartbeater.h File src/kudu/tserver/heartbeater.h: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/heartbeater.h@31 PS9, Line 31: namespace master { : class TabletReportPB; : } What's the change here? http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/heartbeater.cc@378 PS9, Line 378: // Update the tablet statistics if necessary. Obvious from the code. But it's worth commenting on _why_ we're doing this here (i.e. why update tablet stats when a heartbeat thread is about to heartbeat)? Of note, this is inherently wasteful when there are multiple masters: there'll be one heartbeat thread per master, and assuming the heartbeating happens more or less at the same time, all but one of the threads will no-op in this method. Perhaps tablet statistics should be scheduled out-of-band, by the TsTabletManager directly? http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.h@394 PS9, Line 394: mutable rw_spinlock lock_stats_; You sure you don't want to just reuse lock_? http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.h@396 PS9, Line 396: typedef std::unordered_map<std::string, master::ReportedTabletStatsPB> StatsMap; : StatsMap stats_map_; What if ReportedTabletStatsPB were a member of TabletReplica? Then we wouldn't need this map. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@136 PS9, Line 136: to should http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@137 PS9, Line 137: "Should be greater than 'heartbeat_interval_ms'"); Why is this a requirement? Also, could you add a gflag group validator to enforce this? http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1563 PS9, Line 1563: std:: Drop http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1570 PS9, Line 1570: tablet->CountLiveRows(&count); This can fail, right? What should we do if it does? Skip the tablet? Warn? Both? Something else? http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1571 PS9, Line 1571: pb.set_row_count(count); : pb.set_disk_size(tablet->OnDiskSize()); This is the part that's likely to evolve the most over time as new statistics are added. I wonder: should we lean into the "metrics aggregation" angle of this patch? If so, ReportedTabletStatsPB should be super generic (maybe a string key/value pair, or a string key + int value), and rather than writing code like this, it should be possible to bind specific tablet-level metrics to the PB. On the other hand, your conversation with Andrew earlier suggested leaning into "interesting statistics aggregation", where the set of interesting values is much smaller than the set of all metrics, and thus ReportedTabletStatsPB defines them individually. Which approach do you think makes more sense? Andrew/Mike, what do you guys think? http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1576 PS9, Line 1576: // Filter the tablets that are newly created or changed. "Include only new tablets or tablets whose stats have changed." http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1578 PS9, Line 1578: one Nit: use 'e' or 'entry' here if there isn't another good name. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1583 PS9, Line 1583: old_stats->disk_size() == new_stats.disk_size() && : old_stats->row_count() == new_stats.row_count()) { Maybe use a protobuf MessageDifferencer here so that this won't need to be updated if/when new stats are added? On the other hand, that might be super expensive, so maybe not worth doing yet. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1592 PS9, Line 1592: // Reset the next update time. : next_update_time_ = MonoTime::Now() + : MonoDelta::FromMilliseconds(FLAGS_update_tablet_stats_interval_ms); : : // Unlock. : lock_stats_.unlock(); : : // Dirty the tablets. : MarkTabletsDirty(dirty_tablets, "The tablet statistics have been changed"); Don't need these comments; the behavior is obvious from the code. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/tserver_path_handlers.cc@421 PS9, Line 421: std:: Drop http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/tserver_path_handlers.cc@423 PS9, Line 423: replica->tablet()->CountLiveRows(&row_count); This should also be set up as a tablet metric, probably using a function gauge. -- 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: 9 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: Wed, 12 Jun 2019 21:12:10 +0000 Gerrit-HasComments: Yes
