helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet statistics ...................................................................... Patch Set 9: (19 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(). Ok, let me have a try. 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. Done 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? Done 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 r Done 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 1.Hmm, should I sum up all of the replicas and displayed in the web UI? 2.Is is acceptable to pick one at random when none of the replicas are LEADER? 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? Done 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 Yes, Andrew's suggestion is good and it really should be here. For the place where computing the tablet stats, I think it's acceptable to let the heartbeat thread drives the tablet stats computation and that's just a simple calculation. Even though the computation is scheduled by TsTabletManager(new thread?), the heartbeat threads can't avoid the lock while accessing the tablet stats. What do you think? 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_? No. This part of the code is independent, not related to the existing ones. Coupling increases the complexity of existing code. In the initial implementation, I placed this part in the class Heartbeater, but later Andrew suggested to put them here and I'm sure he's right. It really should be done from an encapsulation point of view. 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 would ok. 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 Done http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1563 PS9, Line 1563: std:: > Drop Done 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? Hrm, I think skipping the unsupported tablets will be better. Warning is not necessary since it happens so often. 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 statisti I hope to hear more info :) 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." Done 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. Done 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 Yea, I noticed that function early, and I agree it is too expensive because they are compared by serializing objects. 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. Done 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 Done 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 ga I'm sorry I can't catch up your meaning. Could you please explain more? :( -- 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: Thu, 13 Jun 2019 02:57:16 +0000 Gerrit-HasComments: Yes
