helifu 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) 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 Nope, they are pre-replication. And it does not add up. The variable 'metrics_' is a std::unordered_map which will include every replica of this tablet. Hmm, i have a question, is it acceptable to display the first one of 'metrics_'? 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] = metri Done 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 per Done 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 Pending. Maybe we can support incremental metrics. 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 fo Done 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? I'm afraid not. The updated tablets of ReportedTabletPB do not equal to the full/incremental tablet of TabletMetricsPB. 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 report Yea, I have thought about it. But, right now the 'WRITE_OP' doesn't dirty the tablet. That means if we want to make them fully incremental, maybe we need a marker to indicate changes in the data. 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: * Ok. * Before submitting this patch I wanted to write another patch that accurately counted the number of records. But then i thought it would be better if I had your support :) 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 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: 1 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Mon, 27 May 2019 03:30:46 +0000 Gerrit-HasComments: Yes
