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: (15 comments) sorry for the late update. 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) { > 1. Yes, I still think that's valuable, it's just that it'll also be valuabl 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. > The scheduling I'm talking about needn't use a dedicated thread; you could Sorry, I can't catch up your meaning :( 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_? Done http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.h@394 PS9, Line 394: mutable rw_spinlock lock_stats_; > No. Done 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 Done 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_; > ok. Done 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@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? Because we use the heartbeat thread to update the tablet stats. 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? Done http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1570 PS9, Line 1570: tablet->CountLiveRows(&count); > Hrm, I think skipping the unsupported tablets will be better. > Warning is not necessary since it happens so often. Skipping the unsupported tablets is a bad idea since there is not only live row count in stats, I apology. http://gerrit.cloudera.org:8080/#/c/13426/9/src/kudu/tserver/ts_tablet_manager.cc@1570 PS9, Line 1570: tablet->CountLiveRows(&count); > So it's clear to future readers that this was intentional, let's wrap this 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 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()) { > Yea, I noticed that function early, and I agree it is too expensive because 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@423 PS9, Line 423: replica->tablet()->CountLiveRows(&row_count); > This should also be set up as a tablet metric, probably using a function ga 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); > See https://github.com/apache/kudu/blob/master/src/kudu/tablet/tablet_repli 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); > I'm sorry I can't catch up your meaning. Could you please explain more? :( Done -- 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, 19 Jun 2019 12:55:57 +0000 Gerrit-HasComments: Yes
