Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet metrics ...................................................................... Patch Set 1: (9 comments) A couple high level comments: - This patch needs unit testing. - KUDU-2797 also suggested exposing these aggregated metrics as master metrics. Are you going to tackle that in a follow-up? http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/catalog_manager.h@194 PS1, Line 194: uint64_t row_counts(); : uint64_t disk_size(); These are post-replication, right? Because they add up the metrics reported by all the peers? Do we also want to offer pre-replication metrics as per KUDU-2797? http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/catalog_manager.cc@5233 PS1, Line 5233: if (PREDICT_FALSE(it == metrics_.end())) { : metrics_[peer] = metrics; : } else { : it->second = metrics; : } How is this functionally different from just calling metrics_[peer] = metrics? http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/catalog_manager.cc@5245 PS1, Line 5245: uint64_t TabletInfo::row_counts() { : std::lock_guard<simple_spinlock> l(lock_); : return metrics_.empty() ? 0 : metrics_.begin()->second.row_count; : } : : uint64_t TabletInfo::disk_size() { : std::lock_guard<simple_spinlock> l(lock_); : return metrics_.empty() ? 0 : metrics_.begin()->second.disk_size; : } The one caller that exists wants all of this information every time, so perhaps it'd be easier to return a copy of the metrics map directly? http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/master.proto@278 PS1, Line 278: Tablet metrics, this is a full report Does this imply that tablet metrics are only reported on a full report? If so, could you rephrase this sentence to make it more clear? http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/master.proto@278 PS1, Line 278: For backward compatibility, : // it must be optional. You can remove this sentence; we're always going to use 'optional' going forward. http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/master/master.proto@280 PS1, Line 280: optional ReportedTabletMetricsPB tablet_metrics = 5; Why this instead of adding TabletMetricsPB to ReportedTabletPB? http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/heartbeater.cc File src/kudu/tserver/heartbeater.cc: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/heartbeater.cc@77 PS1, Line 77: : DEFINE_int32(heartbeat_metrics_interval_ms, 5000, : "Interval at which the TS metrics to the master."); : TAG_FLAG(heartbeat_metrics_interval_ms, advanced); Rather than using a separate period for reporting metrics and always reporting them, even if they haven't changed, could we make them fully incremental? That is, could we report them as per the usual heartbeat interval, and, if a tablet's metrics have changed, include it in the incremental report along with the changed value? Besides being more efficient (especially in large clusters), this would be way more clear. http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/ts_tablet_manager.cc@1276 PS1, Line 1276: CHECK_OK(replica->tablet()->CountRows(&count_rows)); A couple comments about this: - It may lead to IO if the key reader needs to be initialized for the first time, so we can't just wrap it in CHECK_OK. We need to percolate the failure up the call stack. - The result is inaccurate because it doesn't account for deleted rows. How do you feel about that? http://gerrit.cloudera.org:8080/#/c/13426/1/src/kudu/tserver/ts_tablet_manager.cc@1327 PS1, Line 1327: if (replica->tablet()) { Are these calls to tablet() safe? Or do we need to use shared_tablet() and hold a ref to the Tablet while we operate on it? -- 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: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 24 May 2019 18:52:36 +0000 Gerrit-HasComments: Yes
