helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797: the master aggregates tablet statistics ...................................................................... Patch Set 21: (10 comments) 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 Done 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? Done 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 Done 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_s Done 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? It's about tablet, not replica. So it doesn't matter if it's LEADER or not. 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 LE Done 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 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: } > live_row_count is an optional field; what if we excluded it from the PB? Yes, we can exclude it from the PB. But my concern is how to display this value on the web page. 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 Done 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 d Ah, I missed the function 'Unpublish'. Thank you for reminding me. -- 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: Fri, 19 Jul 2019 05:00:38 +0000 Gerrit-HasComments: Yes
