helifu 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: (17 comments) 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 form Done 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' ? Done 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 ? Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@4295 PS2, Line 4295: hide > hidden Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@5672 PS2, Line 5672: uint64_t on_disk_size = new_stats.on_disk_size(); > Does it make sense to add Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@5685 PS2, Line 5685: // Do not update the metric value when it is invisible, because the update : // operation will trigger the metric to be visible. > I think you can remove this or move this to the generic comment on the 'on Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@5703 PS2, Line 5703: new > newly Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@5705 PS2, Line 5705: if (metrics_->ContainsTabletNoLiveRowCount(tablet_id)) { : if (new_stats.has_live_row_count()) { > nit: combine into one clause? Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/catalog_manager.cc@5721 PS2, Line 5721: } else { : // It is case 1 and the new_stats without 'live_row_count' will keep coming. : } : } else { : // Do not update the metric value when it is invisible. : } > I think you can remove this, maybe moving the comments to the generic comme Done 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 referen Done 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. Done 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'"? Done 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> Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.h@66 PS2, Line 66: IDS > nit: Identifiers ? Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/master/table_metrics.h@68 PS2, Line 68: IDs > nit: Identifiers ? Done 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 Done http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/15071/2/src/kudu/tserver/ts_tablet_manager.cc@1629 PS2, Line 1629: !dirty_tablets.empty() > Good catch! Even if the MarkTabletDirty() doesn't do much if the container :) -- 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-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Mon, 03 Feb 2020 03:31:40 +0000 Gerrit-HasComments: Yes
