Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/14507 )
Change subject: [metrics] Hide metric live_row_count when tablet not support ...................................................................... Patch Set 8: (5 comments) http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/master/catalog_manager.cc@2937 PS8, Line 2937: resp->set_on_disk_size(table->GetMetrics()->on_disk_size->value()); : if (table->GetMetrics()->TableSupportsLiveRowCount()) { : resp->set_live_row_count(table->GetMetrics()->live_row_count->value()); : } : : if (FLAGS_mock_table_metrics_for_testing) { : resp->set_on_disk_size(FLAGS_on_disk_size_for_testing); : resp->set_live_row_count(FLAGS_live_row_count_for_testing); : } : > Maybe clearer as: Done http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/master/table_metrics.cc File src/kudu/master/table_metrics.cc: http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/master/table_metrics.cc@34 PS8, Line 34: "There may be some tablets of the table that not support live row counting, " : "the result is exact if TableSupportsLiveRowCount() return true."); > This second sentence isn't particularly helpful because how could a person Done http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/tablet/diskrowset.cc@761 PS8, Line 761: DCHECK_GE(*count, 0); > This is now always true. We should add a different underflow check prior to Done http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/14507/8/src/kudu/tablet/memrowset.h@260 PS8, Line 260: DCHECK_GE(*count, 0); > True by default. Done http://gerrit.cloudera.org:8080/#/c/14507/6/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/14507/6/src/kudu/util/metrics.h@723 PS6, Line 723: virtual scoped_refptr<Metric> snapshot() const = 0; > I made a mistake in my earlier comment: there's no guarantee that all metri Thanks for your clarification. Done. -- To view, visit http://gerrit.cloudera.org:8080/14507 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a54704d8cbd64a521e65aa3e95bf1a68f7757b7 Gerrit-Change-Number: 14507 Gerrit-PatchSet: 8 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Tue, 22 Oct 2019 10:15:36 +0000 Gerrit-HasComments: Yes
