helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet statistics ...................................................................... Patch Set 20: (33 comments) Thank you for your detailed comments, Adar!^_^ 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 Done 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 t It seems that only leader's stat is enough. 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 p Done 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. Done 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? We can also use "table_id", but it's not enough when we publish metrics without a table name. Here is an example from master's metrics: { "type": "table_stats", "id": "b02f17f3adb646ce821b9df0e9b616ee", "attributes": { "table_name": "impala::ndc_test.goapi_source_Table_u30ly" }, "metrics": [ { "name": "live_row_count", "value": 18 }, { "name": "on_disk_size", "value": 8412072 } ] }, 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 tha 1. A master may be offline for a while, or restarted. -- When a master is offline which triggers the LEADER role change, the tservers should report a full stats. Or every master parses the reports and publish metrics. Is the later more friendly? 2. A tserver may be offline for a while, or restarted. -- It doesn't matter. 3. Replicas may be deleted when a table or range partition is deleted, and (maybe?) they never show up in a report after that. -- Yes, they should never be showed up in the metrics. 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 pop Every master publishes the metrics can help to avoid them, I think. 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 Done 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. Done 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? Yes, it should be. 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? Let me have a try. 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? master_path_handlers.cc Line432/433 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. Done. By the way, for visible tables, there must be metrics. 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? Done http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/table_metrics.cc File src/kudu/master/table_metrics.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/table_metrics.cc@24 PS20, Line 24: "Disk Size" > "Table Size On Disk" Done 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, i Done 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" Done 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." Done 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. Done 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'. Done 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." Done 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()? Done 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 fo I think reporting -1 is the simplest way, and there seems to be no better way. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.cc@838 PS20, Line 838: > Nit: extra space here. Done 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 Emm, the gflag 'update_tablet_stats_interval_ms' has limit the frequency of reports, and can help to release the master's pressure. In addition, I accept Mike's suggestion to report leader's stats only which can also reduce the events. 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 Done 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 reconfigu Done 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. Done 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. Done 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 It's for correctness. The clusters that have a very long heartbeat interval should reconfigure this gflag too. 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: Done 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 w Done 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 d 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: 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: Thu, 18 Jul 2019 07:34:32 +0000 Gerrit-HasComments: Yes
