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 21: (14 comments) There's not enough black box unit test coverage on the aggregated metrics. For example, there should be a 3 master 3 tserver test that verifies the correct values when: 1. Master leadership changes. 2. Tablet leadership changes. 3. Master is restarted. 4. Table is renamed. 5. Table is deleted. http://gerrit.cloudera.org:8080/#/c/13426/21//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13426/21//COMMIT_MSG@13 PS21, 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; You should now remove these from the patch (and the commit message). http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/catalog_manager.h@320 PS21, Line 320: UnRegisterMetrics Nit: UnregisterMetrics http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/catalog_manager.h@368 PS21, Line 368: shared_ptr Why does this need shared ownership? 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(catalog_manager_->master_->metric_registry(), metadata.name()); > We can also use "table_id", but it's not enough when we publish metrics wit Yeah we should probably publish both the table ID and name. But, we should normalize the name so that it makes sense when HMS integration is enabled. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@4171 PS20, Line 4171: if (report.has_stats() && report.has_consensus_state()) { > Every master publishes the metrics can help to avoid them, I think. I'm not talking about leadership changes _between masters_; I'm referring to leadership changes _between tablets_. I talked offline with Andrew and we agreed that we need to better understand how the values of these two aggregated metrics fluctuate. I'm less concerned about live rows: if a replica is elected as leader, it'll have the most up-to-date WAL and thus should have the most correct live row count. However, the on disk size is a _physical_ property of a replica and is affected by things like the number of rowsets and amount of compaction done on that replica. So I expect it'd fluctuate in value after leader elections. What kind of fluctuations can we expect to see? All that said, both aggregated metrics are vulnerable to the same kind of fluctuation: when a master restarts, the aggregated value will be incorrect until all tservers report in. If we make decisions based on the aggregate's value, the decisions will be incorrect. This makes me think that perhaps we should avoid publishing a value until all tservers report. http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/catalog_manager.cc@2676 PS21, Line 2676: // Alter the table name in metrics. : table->RegisterMetrics(master_->metric_registry(), normalized_new_table_name); Shouldn't we just find the existing table's entity and use SetAttribute to change the table_name attribute? http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/catalog_manager.cc@4171 PS21, Line 4171: && report.has_consensus_state() Don't need this anymore? Or perhaps you want to DCHECK that if report.has_stats() is true, the reporting replica is a LEADER? http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/catalog_manager.cc@5321 PS21, Line 5321: // Update the table metrics for the deleted tablets. : UpdateMetrics(tablet->GetStats(), ReportedTabletStatsPB()); How do we know that the deleted tablets were leaders? http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/master/master.proto@246 PS21, Line 246: // Tablet statistics. Should note that this is only included in the report if the replica is a LEADER. http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/tablet/tablet_replica.h File src/kudu/tablet/tablet_replica.h: http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/tablet/tablet_replica.h@308 PS21, Line 308: // The returned 'dirty_tablets' are used to trigger the heartbeat. I meant you should doc what gets written to 'dirty_tablets', not what we do with 'dirty_tablets' after this function is 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@835 PS20, Line 835: } > I think reporting -1 is the simplest way, and there seems to be no better w live_row_count is an optional field; what if we excluded it from the PB? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/tablet/tablet_replica.cc@840 PS20, Line 840: } > Emm, the gflag 'update_tablet_stats_interval_ms' has limit the frequency of Both of those will help, but we need real world (or close to real world) testing to ensure that the impact is what we think it is. http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/tserver/ts_tablet_manager.cc@1585 PS21, Line 1585: MarkTabletsDirty(dirty_tablets, "The tablet statistics have been changed"); We should release try_lock before calling this, as it'll take locks of its own and I can't see a reason to serialize the call to it. In fact, perhaps we should set next_update_time_ on L1573 and release the lock right after that? We don't need it held when calling UpdateTabletStats, do we? http://gerrit.cloudera.org:8080/#/c/13426/21/src/kudu/util/metrics.h File src/kudu/util/metrics.h: PS21: We evaluated destroying entities back in commit 8c23c97b8. At the time we decided it wasn't worth the complexity: instead, we "unpublish" metrics belonging to the entity, and let the entity naturally destroy itself when it goes out of scope. Could we do the same thing here? That is, retain the MetricEntity in memory as part of the TableInfo, unpublish its metrics when the table is deleted, and let the MetricEntity go out of scope with the TableInfo if it's destroyed? -- 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: 21 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 22:58:38 +0000 Gerrit-HasComments: Yes
