Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet statistics ...................................................................... Patch Set 20: (35 comments) http://gerrit.cloudera.org:8080/#/c/13426/20//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13426/20//COMMIT_MSG@13 PS20, Line 13: 1) live row count of the tablet is exposed as metrics on : the tablet server; : 2) live row count of the tablet is exposed on the tablet : server's Web-UI; Could you split this off into a separate patch? It's non-controversial and can be merged while the rest remains under review. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.h@220 PS20, Line 220: std::unordered_map<std::string, tablet::ReportedTabletStatsPB> replica_stats_; Given the low replication factors that we often use (i.e. 3 or 5), do you think it'd be more performant to make this a vector of pairs and iterate through it to find the desired replica? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.h@318 PS20, Line 318: void RegisterMetrics(const std::string& table_name, MetricRegistry* metric_registry); I'd pass metric_registry first. It's a style preference, but generally we pass "boring" singletons and such earlier in the list of args so it's easier to skip them while reading. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.h@365 PS20, Line 365: gscoped_ptr<TableMetrics> metrics_; unique_ptr for new code. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@370 PS20, Line 370: table->RegisterMetrics(metadata.name(), catalog_manager_->master_->metric_registry()); Should this use the normalized table name? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@1785 PS20, Line 1785: table->RegisterMetrics(metadata->name(), master_->metric_registry()); Likewise, normalized table name? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@3938 PS20, Line 3938: tablet->unset_replica_stats(ts_desc->permanent_uuid()); This is one area of the patch that still gives me pause: how do we know that we got the replica metrics lifecycle right? Bear in mind that: 1. A master may be offline for a while, or restarted. 2. A tserver may be offline for a while, or restarted. 3. Replicas may be deleted when a table or range partition is deleted, and (maybe?) they never show up in a report after that. So how do we know that we've got full coverage of all the "unset" events? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@4171 PS20, Line 4171: if (ts_desc->permanent_uuid() == report.consensus_state().leader_uuid()) { If the table-level metrics kept track of which replica UUID was used to populate the metrics for each tablet, we could be more robust about a few things: - Shifting reporting from one leader to another, or from a follower to a leader (if there's no leader). - Not publishing the metric until all tablets have sent at least one report. In general I'd like to better understand how this value fluctuates. At the very least, it seems like when the master restarts there's going to be fluctuation from 0 up to the correct value as more and more tservers report in. Fluctuations are bad as they make the metric less usable for policy decisions. What can we do to avoid them? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@4172 PS20, Line 4172: tablet->table()->UpdateMetrics( The function signature will change with each new aggregated metric. Perhaps we can pass report.stats() and tablet->replica_stats(prev_cstate.leader_uuid()) directly into it, and let it handle the specifics of which metrics exist? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@4174 PS20, Line 4174: tablet->replica_stats(prev_cstate.leader_uuid()).on_disk_size(), Call replica_stats() once and reuse it for both updates here. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@5470 PS20, Line 5470: void TableInfo::RegisterMetrics(const string& table_name, MetricRegistry* metric_registry) { Seems like we should unpublish the entity when the table is deleted, no? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@5473 PS20, Line 5473: attrs["table_name"] = table_name; How does this get updated when a table is renamed? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@5486 PS20, Line 5486: TableMetrics* TableInfo::metrics() { Where is this used? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/master_path_handlers.cc@430 PS20, Line 430: table->metrics() can return nullptr. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/master_path_handlers.cc@434 PS20, Line 434: (*output)["table_row_count"] = table_row_count >= 0 ? table_row_count : -1; -1 isn't very helpful; perhaps "N/A" or something like that? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/table_metrics.cc File src/kudu/master/table_metrics.cc: PS20: > One of the things that makes me hesitant about this is that it's introducin I share your concern, but I don't really see a way around this if Kudu is to provide table-aggregated metrics (vs. relying on metrics collectors to do it). At least now metrics collectors can choose to ignore tablet-level metrics in favor of these aggregations, which should cut the number of collected entities by a few orders of magnitude. Likewise, collectors can ignore these aggregations if they're already aggregating themselves. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/table_metrics.cc@24 PS20, Line 24: "Disk Size" "Table Size On Disk" http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/table_metrics.cc@26 PS20, Line 26: "The disk size of the current table."); "Pre-replication aggregated disk space used by all tablets in this table, including metadata." http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/table_metrics.cc@27 PS20, Line 27: "Live Row count" "Table Live Row count" http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/table_metrics.cc@29 PS20, Line 29: "Number of live rows in the current table." "Pre-replication aggregated number of live rows in this table." http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.h@303 PS20, Line 303: // Return the number of live rows of this tablet replica. Should note what it means for this function to return -1. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.h@306 PS20, Line 306: // Update the tablet stats. Should note what happens to 'dirty_tablets'. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.h@388 PS20, Line 388: // Stats for the tablet replica. Nit: "Cached stats for the tablet replica." http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.cc@822 PS20, Line 822: shared_ptr<Tablet> tablet; : { : std::lock_guard<simple_spinlock> l(lock_); : tablet = tablet_; : } : : if (tablet) { : int64_t live_row_count = -1; : // In case the tablet doesn't support counting live rows, ignore the result. : ignore_result(tablet->CountLiveRows(&live_row_count)); Can this be replaced by a call to TabletReplica::CountLiveRows()? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.cc@835 PS20, Line 835: pb.set_live_row_count(live_row_count); Is it OK to report -1 here? Won't the master aggregate that into e.g. -3 for a RF=3 tablet with one table? Seems like we need more robust handling throughout for live_row_count as it isn't guaranteed to exist. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.cc@838 PS20, Line 838: Nit: extra space here. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.cc@840 PS20, Line 840: dirty_tablets->emplace_back(tablet_id()); Andrew alluded to this in an earlier comment, but we need to understand the impact this has on the master, and on the network traffic between tservers and masters. Previously we were only dirtying tablets for mostly rare events (i.e. LEADER->FOLLOWER or FOLLOWER->LEADER transitions). Now we're dirtying them far more often. For example, a tablet that's being actively written to will pretty much always be dirty. Does this mean the master will issue many more Raft writes? Does it spend more CPU time processing tablet reports? To answer these questions, please deploy this patch on a cluster with some scale and monitor the master's load when there's an active write workload in the cluster. Also monitor the master tablet and see how often it is written to. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager-test.cc File src/kudu/tserver/ts_tablet_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager-test.cc@153 PS20, Line 153: CHECK_OK(writer.Insert(row)); ASSERT_OK http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager-test.cc@325 PS20, Line 325: TEST_F(TsTabletManagerTest, TestTabletStatsReports) { Some lines in this test are terminated with ^M characters. Please reconfigure your editor to avoid inserting these. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager-test.cc@336 PS20, Line 336: GenerateFullTabletReport(&report); Since this function can ASSERT, calls to it should be wrapped in NO_FATALS. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager-test.cc@376 PS20, Line 376: InsertTestRows(replica1->tablet(), kCount); Wrap in NO_FATALS. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager.cc@235 PS20, Line 235: GROUP_FLAG_VALIDATOR(update_tablet_stats_interval_ms, ValidateUpdateTabletStatsInterval); I'm worried that this will break upgrades for clusters that have configured a very long heartbeat interval (i.e. 5s). Is it necessary for correctness, or just performance? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/ts_tablet_manager.cc@1563 PS20, Line 1563: if (!lock_update_.try_lock()) { Wrap the try_lock in an RAII guard: std::unique_lock<rw_spinlock> try_lock(lock_update_, std::try_to_lock); if (try_lock.owns_lock()) { return; } ... http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tserver/tserver_path_handlers.cc@418 PS20, Line 418: output->Set("tablet_row_count", replica->CountLiveRows()); Should we have some special handling if CountLiveRows() return -1? Should we emit a different value, like "N/A" or something? http://gerrit.cloudera.org:8080/#/c/13426/20/www/table.mustache File www/table.mustache: http://gerrit.cloudera.org:8080/#/c/13426/20/www/table.mustache@35 PS20, Line 35: <tr><td>Live Row Count:</td><td>{{table_row_count}}</td></tr> : <tr><td>On-disk Size:</td><td>{{table_disk_size}}</td></tr> Maybe add "Aggregated" to these so it's clear that they're summed up from different sources? -- 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: 20 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, 17 Jul 2019 06:30:53 +0000 Gerrit-HasComments: Yes
