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: (5 comments) I took another look. Looks good to me overall, just a few additional nits. 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@5672 PS2, Line 5672: uint64_t on_disk_size = new_stats.on_disk_size(); Does it make sense to add DCHECK(new_stats.has_on_disk_size()) ? 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 disk size' metric visibility above. 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? i.e. replace if (conditionA) { if (conditionB) { ... } } with if (conditionA && conditionB) { ... } 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 comment above regarding the visibility of the 'live row count' metric. 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 is empty, it iterates over all heartbeater threads, acquiring dirty_tables_lock_. With this change, it's no longer doing the unnecessary work. -- 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 <hzhel...@corp.netease.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <verjov...@cloudera.com> Gerrit-Comment-Date: Tue, 28 Jan 2020 00:47:25 +0000 Gerrit-HasComments: Yes
