Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15071 )
Change subject: KUDU-2986 p3: hide the values while there is old version ...................................................................... Patch Set 2: (12 comments) I took a quick first look, and this looks reasonable. I'll take a closer look tomorrow morning. http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/client/table_statistics-internal.h File src/kudu/client/table_statistics-internal.h: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/client/table_statistics-internal.h@37 PS2, Line 37: : on_disk_size_(std::move(on_disk_size)) : , live_row_count_(std::move(live_row_count)) { nit: aside from the essence of this change, I think the way how it was formatted in the former version was closer to the style guideline http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.h@336 PS2, Line 336: InvalidMetrics Rename into 'InvalidateMetrics' ? http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@4285 PS2, Line 4285: its own it owns ? http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@4295 PS2, Line 4295: hide hidden http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@5703 PS2, Line 5703: new newly http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/master-test.cc@1983 PS2, Line 1983: tables[0] nit: for the ease of referencing this single table, maybe declare a reference and use it throughout the code, something like const auto& table = tables[0]; http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/master-test.cc@1986 PS2, Line 1986: tablets.back() Add an assert to make sure 'tablets' is not empty. http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/master_path_handlers.cc@502 PS2, Line 502: we need to show them explicitly This reads a bit confusing. Should it be "show their values as 'N/A'"? http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.h File src/kudu/master/table_metrics.h: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.h@20 PS2, Line 20: #include <stddef.h> nit: use <cstddef> http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.h@66 PS2, Line 66: IDS nit: Identifiers ? http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.h@68 PS2, Line 68: IDs nit: Identifiers ? http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.cc File src/kudu/master/table_metrics.cc: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.cc@52 PS2, Line 52: , nit: correct the spacing -- To view, visit http://gerrit.cloudera.org:8080/15071 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 Gerrit-Change-Number: 15071 Gerrit-PatchSet: 2 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 24 Jan 2020 05:22:02 +0000 Gerrit-HasComments: Yes
