Alexey Serbin 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'. 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. 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. 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 ASSERT_NE(-1, FLAGS_live_row_count_for_testing); 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. 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 default string constructor produces an empty string. 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 catalog_manager, so name it like catalog_manager_support_live_row_count. 'for_testing' is redundant -- it should be in description and the flag is hidden. 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 only. 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 collection when it's enabled? If not, please add one. -- 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 02:50:40 +0000 Gerrit-HasComments: Yes
