helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet metrics ...................................................................... Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/13426/6//COMMIT_MSG Commit Message: PS6: > Could you update this to use "stats" too? ok. Do you plan on using the follower stats at all? -> Hrm, it's better than nothing. But actually, I haven't decided yet. http://gerrit.cloudera.org:8080/#/c/13426/6//COMMIT_MSG@15 PS6, Line 15: And on this basis, we can expose this information to Impala > Question about how you intend on using this: It's 5 seconds. Sure, we should also expose these statistics to Impala through some API. http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/master/catalog_manager.cc@5257 PS6, Line 5257: const auto it = replica_stats_.find(replica_id); : if (it != replica_stats_.end()) { : return it->second; : } > nit: This may be simpler if you use FindOrNull, e.g. Done http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/heartbeater.h File src/kudu/tserver/heartbeater.h: PS6: > nit: now that this hasn't changed, could you revert this file? Done http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/heartbeater.cc@559 PS6, Line 559: // Update the tablet statistics if necessary. > nit: maybe consider putting this by L442. DoHeartbeat seems to contain the Done http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/ts_tablet_manager.cc@1289 PS6, Line 1289: StatsMap::const_iterator it = stats_map_.find(replica->tablet_id()); : if (it != stats_map_.end()) { : reported_tablet->mutable_stats()->CopyFrom(*it->second); : } > FindOrNull might be convenient here too. Done http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/ts_tablet_manager.cc@1545 PS6, Line 1545: // Check if the time is up. : { : shared_lock<rw_spinlock> l(lock_stats_); : if (MonoTime::Now() < next_update_time_) { : return; : } : } : : // Only one thread is allowed to update at the same time. : if (!lock_stats_.try_lock()) { : return; : } > Rather than locking twice, what do you think about something like: Done http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/ts_tablet_manager.cc@1568 PS6, Line 1568: unique_ptr<ReportedTabletStatsPB> pb(new ReportedTabletStatsPB()) > Do these need to be heap-allocated? Could we create the ReportedTableStatsP The reason why I use heap-allocated memory is that the compiler will complain the second object is 'incomplete type' for 'std::unordered_map<std::string, master::ReportedTabletStatsPB>' while the ReportedTabletStatsPB is forward declaration, but when I add '#include "kudu/master/master.pb.h"' in header file, the IWYU will complain the '#include "kudu/master/master.pb.h"' should be removed to .cc file. I don't know if it's my local system's problem. :( http://gerrit.cloudera.org:8080/#/c/13426/6/src/kudu/tserver/ts_tablet_manager.cc@1600 PS6, Line 1600: for (const auto& tablet_id : dirty_tablets) { : MarkTabletDirty(tablet_id, "The tablet statistics have been changed"); : } > Hrm, indeed. I think 'in batch' would be better than 'one by one'. What do you think? http://gerrit.cloudera.org:8080/#/c/13426/6/www/table.mustache File www/table.mustache: http://gerrit.cloudera.org:8080/#/c/13426/6/www/table.mustache@35 PS6, Line 35: <tr><td>Disk Size:</td><td>{{table_disk_size}}</td></tr> > Since this is also a UI change, if you can, could you manually test on a sm Sure, I have tested table stats on my local cluster. -- 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: 6 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 03:23:50 +0000 Gerrit-HasComments: Yes
