helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/14567 )
Change subject: KUDU-2986 p1: adjust the output of "table statistics" from 0 -> N/A ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/14567/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14567/2//COMMIT_MSG@8 PS2, Line 8: > Write a meaningful description: what is contained in 'KUDU-2986 p1'. Done http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@805 PS2, Line 805: Support > What supports what? Please write a meaningful sentence here. Done http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@809 PS2, Line 809: Not support live row count. > What doesn't supports what? Please write a meaningful sentence here. Done http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/client/client-test.cc@813 PS2, Line 813: ASSERT_EQ(-1, statistics->live_row_count()); > How do we know FLAGS_live_row_count_for_testing is not -1? Please add Done http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/client.h@974 PS1, Line 974: /// -1 is returned if the table doesn't support live_row_count. > Move this under the @return section. Done http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h File src/kudu/client/table_statistics-internal.h: http://gerrit.cloudera.org:8080/#/c/14567/1/src/kudu/client/table_statistics-internal.h@45 PS1, Line 45: = "" > While you are at it, please drop this assignment since it's redundant. The Done http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc@278 PS2, Line 278: support_live_row_count_for_testing > Please follow the convention for naming flags in this file. This is catalo Done http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/master/catalog_manager.cc@279 PS2, Line 279: Whether to enable mock support live row count for testing. > Whether to enable mock live row count statistic for tables. For testing onl Done http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/14567/2/src/kudu/tools/kudu-tool-test.cc@5574 PS2, Line 5574: TEST_F(ToolTest, TestGetTableStatisticsLiveRowCountNotSupported) { > Is there a test to verify functionality of the live row count stats collect Yep, I think below two test cases are enough: https://github.com/apache/kudu/blob/0870259456f785c4efdf5826350166d8d6dff5cc/src/kudu/integration-tests/ts_tablet_manager-itest.cc#L699 https://github.com/apache/kudu/blob/0870259456f785c4efdf5826350166d8d6dff5cc/src/kudu/master/master-test.cc#L1778 -- To view, visit http://gerrit.cloudera.org:8080/14567 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I82c47f5ac07cf63d455c0720614c5383bfa88ca3 Gerrit-Change-Number: 14567 Gerrit-PatchSet: 2 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Wed, 30 Oct 2019 07:33:29 +0000 Gerrit-HasComments: Yes
