Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet metrics ...................................................................... Patch Set 5: (5 comments) Overall this looks great. Thanks for implementing this, Lifu! A high-level concern I have is that while the master aggregates the count and size statistics for all replicas of each tablet, it only reports on a random replica (the first in the hash table). One downside to that approach is that the counts may be unstable if the consensus group membership is unstable. Since counts, and all logical stats (admittedly, not physical stats like on-disk size) can only change when there are writes through the leader, I think it would improve stability of the stats to prefer using the stats reported by the most recent leader of each tablet. The downside to *only* accepting leader stats is that if some tablets are only available for read (due to lack of quorum / server downtime), and the master is restarted, then it would not be able to use any stats for that tablet. So then a hybrid approach would be needed, maybe something like: When reporting table stats (i.e., through the web UI, or eventually to Impala), for each tablet: - If a leader has ever reported into this master, use the stats for the most recent leader that has reported (highest-term leader) - If a leader has never reported, use the stats from the first replica (same approach used now, which is basically a consistent random selection) I think this would give us a reasonable level of consistency, without which I worry we might run into difficult-to-debug behavior if stats from potentially extremely lagging replicas might be used for join strategy decisions. It should also be simple to implement using the ConsensusStatePB reported by the tablet. What do you think? 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 +1 Maybe a better name would be first_replica_stats() See my other comment about this in general, though. 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 co +1, statistics seems more descriptive of what we're building in here 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@77 PS5, Line 77: void SetUpdateTimeForTests(const MonoTime& time); nit: If possible, make the ForTests() stuff private and add the test as a friend with FRIEND_TEST 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@319 PS5, Line 319: lock_.try_lock() nit: this would be more resistant to bugs caused by future code changes if we use unique_lock: std::unique_lock<rw_spinlock> l(lock_, std::defer_lock); if (!l.try_lock()) return; Just wondering, what could cause multiple threads to end up in this method? http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/ts_tablet_manager-test.cc File src/kudu/tserver/ts_tablet_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/13426/5/src/kudu/tserver/ts_tablet_manager-test.cc@338 PS5, Line 338: 1. nit: when formatting numbering, please add a space after the dot, like: // 1. Create two tablets. -- 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 <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Tue, 11 Jun 2019 07:29:07 +0000 Gerrit-HasComments: Yes
