[kudu-CR] KUDU-2986 p3: hide the values while there is old version
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15071 ) Change subject: KUDU-2986 p3: hide the values while there is old version .. KUDU-2986 p3: hide the values while there is old version The old version will not report statistics in the heartbeat msg, so the aggregated values are not accurate and we need to hide them. Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 Reviewed-on: http://gerrit.cloudera.org:8080/15071 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_statistics-internal.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master_path_handlers.cc M src/kudu/master/table_metrics.cc M src/kudu/master/table_metrics.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc 13 files changed, 233 insertions(+), 95 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- 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: merged Gerrit-Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 Gerrit-Change-Number: 15071 Gerrit-PatchSet: 4 Gerrit-Owner: helifu Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Reviewer: helifu
[kudu-CR] KUDU-2986 p3: hide the values while there is old version
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 3: Thank you for fixing the issue! -- 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: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Reviewer: helifu Gerrit-Comment-Date: Mon, 03 Feb 2020 06:01:24 + Gerrit-HasComments: No
[kudu-CR] KUDU-2986 p3: hide the values while there is old version
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 3: Code-Review+2 -- 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: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Reviewer: helifu Gerrit-Comment-Date: Mon, 03 Feb 2020 06:00:59 + Gerrit-HasComments: No
[kudu-CR] KUDU-2986 p3: hide the values while there is old version
Hello Alexey Serbin, Kudu Jenkins, Volodymyr Verovkin, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15071 to look at the new patch set (#3). Change subject: KUDU-2986 p3: hide the values while there is old version .. KUDU-2986 p3: hide the values while there is old version The old version will not report statistics in the heartbeat msg, so the aggregated values are not accurate and we need to hide them. Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_statistics-internal.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master_path_handlers.cc M src/kudu/master/table_metrics.cc M src/kudu/master/table_metrics.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc 13 files changed, 233 insertions(+), 95 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15071/3 -- 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: newpatchset Gerrit-Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 Gerrit-Change-Number: 15071 Gerrit-PatchSet: 3 Gerrit-Owner: helifu Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin Gerrit-Reviewer: helifu
[kudu-CR] KUDU-2986 p3: hide the values while there is old version
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
> nit: use
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2986 p3: hide the values while there is old version
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
nit: use
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 24 Jan 2020 05:22:02 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2986 p3: hide the values while there is old version
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15071 to look at the new patch set (#2). Change subject: KUDU-2986 p3: hide the values while there is old version .. KUDU-2986 p3: hide the values while there is old version The old version will not report statistics in the heartbeat msg, so the aggregated values are not accurate and we need to hide them. Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_statistics-internal.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master_path_handlers.cc M src/kudu/master/table_metrics.cc M src/kudu/master/table_metrics.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tserver/ts_tablet_manager-test.cc M src/kudu/tserver/ts_tablet_manager.cc 13 files changed, 240 insertions(+), 96 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15071/2 -- 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: newpatchset Gerrit-Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 Gerrit-Change-Number: 15071 Gerrit-PatchSet: 2 Gerrit-Owner: helifu Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2986 p3: hide the values while there is old version
helifu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15071 Change subject: KUDU-2986 p3: hide the values while there is old version .. KUDU-2986 p3: hide the values while there is old version The old version will not report statistics in the heartbeat msg, so the aggregated values are not accurate and we need to hide them. Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 --- M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/table_statistics-internal.h M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/master/master-test.cc M src/kudu/master/master_path_handlers.cc M src/kudu/master/table_metrics.cc M src/kudu/master/table_metrics.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tserver/ts_tablet_manager.cc 12 files changed, 230 insertions(+), 91 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15071/1 -- 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: newchange Gerrit-Change-Id: I691bb8c447c878471ed795028f0aab89a39bd946 Gerrit-Change-Number: 15071 Gerrit-PatchSet: 1 Gerrit-Owner: helifu
