You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2022/04/27 01:13:26 UTC
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18451
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
[client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it is currently lack of per-session metrics
about how many rows get ignored vs modified. This patch implement
per-session metrics as WriteOpMetrics that is sent to client within
WriteResponsePB.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tserver/tserver.proto
8 files changed, 88 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/1
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#8).
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
[client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it was lacking the per-session metrics about how
many rows get ignored vs modified. This patch implements the per-session
metrics by introducing a new ResourceMetricsPB field into the
WriteResponsePB that's populated in every response sent back to the
client.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tserver/tserver.proto
11 files changed, 135 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/8
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 11:
Thank you for the review, Alexey!
> Patch Set 10: Code-Review+2
>
> Thank you for the contribution!
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 11
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 30 Apr 2022 20:07:01 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h@53
PS2, Line 53: #include "kudu/tablet/ops/op.h"
> Unfortunately, this file isn't among the files included into the set of hea
After some consideration, I think that switching to the ResourceMetrics in the C++ client API is the best option.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Apr 2022 21:37:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 4:
(5 comments)
Patch set 4 change the implementation to use the existing ResourceMetricsPB.
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h@53
PS2, Line 53:
> Sorry, I just read this message. Will fix this next.
Done. We're not using kudu::tablet::OpMetrics anymore.
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h@2570
PS2, Line 2570: const ResourceMetrics& GetWriteOpMe
> Could this method be 'const' (like the client() method below)?
This is deleted in patch set 4.
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/error_collector.h
File src/kudu/client/error_collector.h:
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/error_collector.h@56
PS2, Line 56:
> nit: in Kudu we use google's C++ code style (https://google.github.io/style
Done
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/error_collector.h@56
PS2, Line 56: al ~ErrorCollec
> nit: I guess 'IncrementWriteMetrics' or 'UpdateWriteMetrics' might be a bet
Replaced with KuduSession::Data::UpdateWriteOpMetrics.
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc@318
PS1, Line 318:
: TRACE("FINISH: Updating metrics");
:
: if (auto* metrics = state_->tablet_replica()->tablet()->metrics();
: PREDICT_TRUE(metrics != nullptr)) {
: // TODO(unknown): should we change this so it's actually incremented by the
: // Tablet code itself instead of this wrapper code?
> Yep, it seems the approach in PS2 has addressed that concern.
This is fixed now. The key is to set the response metrics before calling FinishApplyingOrAbort().
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Apr 2022 01:54:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@7
PS4, Line 7: WriteOpMetrics
This should be changed to "ResourceMetricsPB"
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Apr 2022 01:58:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h@53
PS2, Line 53: #include "kudu/tablet/ops/op.h"
> After some consideration, I think that switching to the ResourceMetrics in
Sorry, I just read this message. Will fix this next.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Apr 2022 21:49:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#6).
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
[client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it was lacking the per-session metrics about how
many rows get ignored vs modified. This patch implements the per-session
metrics by introducing a new ResourceMetricsPB field into the
WriteResponsePB that's populated in every response sent back to the
client.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tserver/tserver.proto
10 files changed, 129 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/6
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#10).
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
[client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it was lacking the per-session metrics about how
many rows get ignored vs modified. This patch implements the per-session
metrics by introducing a new ResourceMetricsPB field into the
WriteResponsePB that's populated in every response sent back to the
client.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tserver/tserver.proto
11 files changed, 138 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/10
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/client/client-test.cc@2939
PS7, Line 2939: const KuduSession* s
> Done
OK, thanks: const pointer will do here as well -- it's a just a test after all.
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/integration-tests/exactly_once_writes-itest.cc
File src/kudu/integration-tests/exactly_once_writes-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/integration-tests/exactly_once_writes-itest.cc@178
PS7, Line 178: esponse.has_resource_metrics()) {
> You are right. The rest of responses actually match.
Thanks!
http://gerrit.cloudera.org:8080/#/c/18451/9/src/kudu/integration-tests/exactly_once_writes-itest.cc
File src/kudu/integration-tests/exactly_once_writes-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18451/9/src/kudu/integration-tests/exactly_once_writes-itest.cc@183
PS9, Line 183: response.release_resource_metrics();
I didn't notice that in previous review cycle, but instead here should be
response.clear_resource_metrics();
Otherwise, it's a memory leak -- release returns the underlying structure along with transferring the ownership of the allocated structure to the caller.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Apr 2022 21:00:51 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc@318
PS1, Line 318: resp_metrics->set_successful_inserts(op_m.successful_inserts);
: resp_metrics->set_insert_ignore_errors(op_m.insert_ignore_errors);
: resp_metrics->set_successful_upserts(op_m.successful_upserts);
: resp_metrics->set_successful_updates(op_m.successful_updates);
: resp_metrics->set_update_ignore_errors(op_m.update_ignore_errors);
: resp_metrics->set_successful_deletes(op_m.successful_deletes);
: resp_metrics->set_delete_ignore_errors(op_m.delete_ignore_errors);
> Yes, we want to accumulate per-session metrics on the client side. Does the
Yep, it seems the approach in PS2 has addressed that concern.
As for the crash while running the test, I'd start with understanding what went wrong in that particular case. Maybe, run the test scenario where it segfaults under debugger and see what's going on. Probably, some field is nullptr while trying do dereference it. Going through the stack of the thread segfaulting and the stacks of other threads could give you some hints: the WriteOpState::response_ pointer field isn't assigned as needed, etc.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Apr 2022 23:04:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#9).
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
[client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it was lacking the per-session metrics about how
many rows get ignored vs modified. This patch implements the per-session
metrics by introducing a new ResourceMetricsPB field into the
WriteResponsePB that's populated in every response sent back to the
client.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tserver/tserver.proto
11 files changed, 138 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/9
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
[client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it was lacking the per-session metrics about how
many rows get ignored vs modified. This patch implements the per-session
metrics by introducing a new ResourceMetricsPB field into the
WriteResponsePB that's populated in every response sent back to the
client.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Reviewed-on: http://gerrit.cloudera.org:8080/18451
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <al...@apache.org>
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tserver/tserver.proto
11 files changed, 138 insertions(+), 2 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 11
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/batcher.cc@971
PS1, Line 971: AddWriteMetrics
> Isn't this overwriting the metrics instead of merging them? From the descr
You're right, this should sum rather than override. Fixed in patch set 2.
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1:
> It seems the successful_upserts metric isn't covered yet. Does it make sen
Done
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2566
PS1, Line 2566:
> Need to document this the way the other methods around are documented: the
Done
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2567
PS1, Line 2567: /// Get the total of write operation metrics during the lifetime of a session.
> Should this method be marked as constant one?
Fixed in patch set 3.
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2567
PS1, Line 2567: /// Ge
> nit: is it possible to drop the 'kudu::' part given the outer namespace is
Done
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/error_collector.h
File src/kudu/client/error_collector.h:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/error_collector.h@56
PS1, Line 56:
> Why not to use constant reference for the parameter? The argument isn't mo
Done
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc@318
PS1, Line 318: resp_metrics->set_successful_inserts(op_m.successful_inserts);
: resp_metrics->set_insert_ignore_errors(op_m.insert_ignore_errors);
: resp_metrics->set_successful_upserts(op_m.successful_upserts);
: resp_metrics->set_successful_updates(op_m.successful_updates);
: resp_metrics->set_update_ignore_errors(op_m.update_ignore_errors);
: resp_metrics->set_successful_deletes(op_m.successful_deletes);
: resp_metrics->set_delete_ignore_errors(op_m.delete_ignore_errors);
> Instead, would it make sense to report back on the metrics for this particu
Yes, we want to accumulate per-session metrics on the client side. Does the change in patch set 2 (ErrorCollector::AddWriteMetrics) address this concern?
On different issue, I can't seem to run client-test manually myself. I always get this error:
I0427 14:43:01.165669 21595 catalog_manager.cc:1375] Loading table and tablet metadata into memory...
I0427 14:43:01.166294 21595 catalog_manager.cc:1384] Initializing Kudu cluster ID...
*** Aborted at 1651095781 (unix time) try "date -d @1651095781" if you are using GNU date ***
PC: @ 0x7febcb8f87f3 kudu::tserver::WriteResponsePB::_internal_mutable_metrics()
*** SIGSEGV (@0x10) received by PID 21364 (TID 0x7feb98f9b700) from PID 16; stack trace: ***
@ 0x7febc9359f71 google::(anonymous namespace)::FailureSignalHandler()
@ 0x7febca7ca980 (unknown)
@ 0x7febcb8f87f3 kudu::tserver::WriteResponsePB::_internal_mutable_metrics()
@ 0x7febcb8f884e kudu::tserver::WriteResponsePB::mutable_metrics()
@ 0x7febcb8f48cc kudu::tablet::WriteOp::Finish()
@ 0x7febcb8e31c4 kudu::tablet::OpDriver::Finalize()
@ 0x7febcb8e2bf1 kudu::tablet::OpDriver::ApplyTask()
@ 0x7febcb8e1b51 _ZZN4kudu6tablet8OpDriver10ApplyAsyncEvENKUlvE_clEv
@ 0x7febcb8e490f _ZNSt17_Function_handlerIFvvEZN4kudu6tablet8OpDriver10ApplyAsyncEvEUlvE_E9_M_invokeERKSt9_Any_data
@ 0x7febccc00796 std::function<>::operator()()
@ 0x7febca07bc18 kudu::ThreadPool::DispatchThread()
@ 0x7febca07c501 _ZZN4kudu10ThreadPool12CreateThreadEvENKUlvE_clEv
@ 0x7febca07dc15 _ZNSt17_Function_handlerIFvvEZN4kudu10ThreadPool12CreateThreadEvEUlvE_E9_M_invokeERKSt9_Any_data
@ 0x7febccc00796 std::function<>::operator()()
@ 0x7febca06d36b kudu::Thread::SuperviseThread()
@ 0x7febca7bf6db start_thread
@ 0x7febc7fb661f clone
[1] 21364 segmentation fault ./bin/client-test
Is there any mistakes in how I initialize WriteOpMetrics here?
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tserver/tserver.proto@166
PS1, Line 166: WriteOpMetrics
> The convention in the Kudu is to add the 'PB' suffix for PB structures.
Done
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tserver/tserver.proto@167
PS1, Line 167: optional uint64 successful_inserts = 1;
: optional uint64 insert_ignore_errors = 2;
: optional uint64 successful_upserts = 3;
: optional uint64 successful_updates = 4;
: optional uint64 update_ignore_errors = 5;
: optional uint64 successful_deletes = 6;
: optional uint64 delete_ignore_errors = 7;
: optional uint64 commit_wait_duration_usec = 8;
> I guess all these should be optional, especially in the context of eventual
Done
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Apr 2022 21:46:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#5).
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
[client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it was lacking the per-session metrics about how
many rows get ignored vs modified. This patch implements the per-session
metrics by introducing a new ResourceMetricsPB field into the
WriteResponsePB that's populated in every response sent back to the
client.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tserver/tserver.proto
9 files changed, 121 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/5
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h@53
PS2, Line 53: #include "kudu/tablet/ops/op.h"
Unfortunately, this file isn't among the files included into the set of headers coming with the Kudu C++ client, and it has too many dependencies to be added into that set.
I guess you'd need to add a new structure somewhere in the 'exported' header files in src/kudu/client or src/kudu/common (see https://github.com/apache/kudu/blob/50b1cc45f9fde3deac5aa0fef216f4950246b2c9/src/kudu/client/CMakeLists.txt#L184-L213). That new structure would mirror tablet::OpMetrics (but please use 64-bit counters instead of 32-bit ones in tablet::OpMetrics). Probably, src/kudu/client/resource_metrics.h is a good place to add that.
Alternatively, you could try to use ResourceMetrics as it's done for scan operations (again, see src/kudu/client/resource_metrics.{h,cc} and its usage throughout the Kudu C++ code, like in https://github.com/apache/kudu/blob/50b1cc45f9fde3deac5aa0fef216f4950246b2c9/src/kudu/client/scanner-internal.cc#L234-L248).
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h@2570
PS2, Line 2570: tablet::OpMetrics GetWriteMetrics()
Could this method be 'const' (like the client() method below)?
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/error_collector.h
File src/kudu/client/error_collector.h:
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/error_collector.h@56
PS2, Line 56: tserver::WriteOpMetricsPB &
nit: in Kudu we use google's C++ code style (https://google.github.io/styleguide/cppguide.html), so the asterisk should be stick to the type, not the name of the parameter.
http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/error_collector.h@56
PS2, Line 56: AddWriteMetrics
nit: I guess 'IncrementWriteMetrics' or 'UpdateWriteMetrics' might be a better name here?
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Apr 2022 21:31:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 10: Code-Review+2
Thank you for the contribution!
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 30 Apr 2022 05:00:49 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 5:
(2 comments)
It might be a red herring, but Jenkins pre-commit tests report on failures in ExactlyOnceSemanticsITest.TestWritesWithExactlyOnceSemanticsWithCrashyNodes: http://jenkins.kudu.apache.org/job/kudu-gerrit/25432/
I ran the new bits built with the source with this patch saw it became significantly flaky, while dist-test dashboard hasn't reported anything like that for this test for a long time: http://dist-test.cloudera.org:8080/
The logs for the DEBUG build could be downloaded from here: http://dist-test.cloudera.org/job?job_id=jenkins-slave.1651169101.3480026
Did you mind taking a look at what's going on with that test?
Thanks!
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc@973
PS4, Line 973: }
> This follow an example in Batcher::CheckForFinishedFlush() on how to access
Yup, in Batcher::CheckForFinishedFlush() the lock was necessary to guard access to the state variables.
http://gerrit.cloudera.org:8080/#/c/18451/5/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/18451/5/src/kudu/client/batcher.cc@970
PS5, Line 970: sp::shared_ptr<KuduSession> session = weak_session_.lock();
: if (session) {
C+17 nit: as a best practice to keep variables local only to the relevant code scope, you could move the 'session' under the if() clause:
if (sp::shared_ptr<KuduSession> session = weak_session_.lock()) {
session->data_->UpdateWriteOpMetrics(rpc.resp().resource_metrics());
}
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Apr 2022 21:34:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 6:
(2 comments)
ExactlyOnceSemanticsITest.TestWritesWithExactlyOnceSemanticsWithCrashyNodes failed due to mismatch Response content.
I'm trying to fix it in patch set 6 by dumping the write operation metrics to response object twice: on Prepare and on Finish.
http://gerrit.cloudera.org:8080/#/c/18451/5/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/18451/5/src/kudu/client/batcher.cc@970
PS5, Line 970: if (sp::shared_ptr<KuduSession> session = weak_session_.lock()) {
: session->dat
> C+17 nit: as a best practice to keep variables local only to the relevant c
Done
http://gerrit.cloudera.org:8080/#/c/18451/6/src/kudu/tablet/ops/write_op.h
File src/kudu/tablet/ops/write_op.h:
http://gerrit.cloudera.org:8080/#/c/18451/6/src/kudu/tablet/ops/write_op.h@264
PS6, Line 264: void DumpWriteOpToResponse(consensus::DriverType type);
Forgot to write documentation here.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Apr 2022 23:14:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#2).
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
[client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it is currently lack of per-session metrics
about how many rows get ignored vs modified. This patch implement
per-session metrics as WriteOpMetrics that is sent to client within
WriteResponsePB.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tserver/tserver.proto
8 files changed, 96 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/2
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 2
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 7:
Aside from the unrelated test failures, the IWYU build isn't yet happy:
>>> Fixing #includes in '/home/jenkins-slave/workspace/kudu-master/2/src/kudu/integration-tests/exactly_once_writes-itest.cc'
@@ -28,6 +28,7 @@
#include <glog/logging.h>
#include <gtest/gtest.h>
+#include "kudu/common/row_operations.pb.h"
#include "kudu/common/schema.h"
#include "kudu/common/wire_protocol-test-util.h"
#include "kudu/common/wire_protocol.h"
IWYU would have edited 1 files on your behalf.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Apr 2022 16:01:04 +0000
Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/batcher.cc@971
PS1, Line 971: SetWriteMetrics
Isn't this overwriting the metrics instead of merging them? From the description in the commit message, I'd expect the per-session metrics are being merged upon completion of each batch: i.e. corresponding counters are summed up, not simply set.
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS1:
It seems the successful_upserts metric isn't covered yet. Does it make sense to add a scenario for that as well?
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2566
PS1, Line 2566:
Need to document this the way the other methods around are documented: the added docs will appear in the auto-generated API reference.
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2567
PS1, Line 2567: kudu::
nit: is it possible to drop the 'kudu::' part given the outer namespace is 'kudu' already?
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2567
PS1, Line 2567: kudu::tablet::OpMetrics GetWriteMetrics();
Should this method be marked as constant one?
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/error_collector.h
File src/kudu/client/error_collector.h:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/error_collector.h@56
PS1, Line 56: tserver::WriteOpMetrics
Why not to use constant reference for the parameter? The argument isn't moved, so using constant reference might save some extra copying, IIUC.
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tablet/ops/write_op.cc@318
PS1, Line 318: resp_metrics->set_successful_inserts(op_m.successful_inserts);
: resp_metrics->set_insert_ignore_errors(op_m.insert_ignore_errors);
: resp_metrics->set_successful_upserts(op_m.successful_upserts);
: resp_metrics->set_successful_updates(op_m.successful_updates);
: resp_metrics->set_update_ignore_errors(op_m.update_ignore_errors);
: resp_metrics->set_successful_deletes(op_m.successful_deletes);
: resp_metrics->set_delete_ignore_errors(op_m.delete_ignore_errors);
Instead, would it make sense to report back on the metrics for this particular operation instead of total number? It seems we are interested in accumulating per-session metrics on the client side, not just report on per-server metrics accumulated on some particular tablet replica, no?
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tserver/tserver.proto@166
PS1, Line 166: WriteOpMetrics
The convention in the Kudu is to add the 'PB' suffix for PB structures.
http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tserver/tserver.proto@167
PS1, Line 167: required int32 successful_inserts = 1;
: required int32 insert_ignore_errors = 2;
: required int32 successful_upserts = 3;
: required int32 successful_updates = 4;
: required int32 update_ignore_errors = 5;
: required int32 successful_deletes = 6;
: required int32 delete_ignore_errors = 7;
: required uint64 commit_wait_duration_usec = 8;
I guess all these should be optional, especially in the context of eventually migrating to protobuf3. And at least in the current code, commit_wait_duration_usec is optional since it's populated only in some certain cases.
Also, the corresponding tablet server's counters are 64 bit metrics, not 32 bit, so these counters should be 64 bit as well.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 27 Apr 2022 04:30:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#7).
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
[client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it was lacking the per-session metrics about how
many rows get ignored vs modified. This patch implements the per-session
metrics by introducing a new ResourceMetricsPB field into the
WriteResponsePB that's populated in every response sent back to the
client.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tablet/ops/write_op.h
M src/kudu/tserver/tserver.proto
11 files changed, 134 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/7
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#4).
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
[client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it is currently lack of per-session metrics
about how many rows get ignored vs modified. This patch implement
per-session metrics as ResourceMetricsPB that is sent to client within
WriteResponsePB.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/resource_metrics.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tserver/tserver.proto
9 files changed, 120 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/4
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/client/client-test.cc@2939
PS7, Line 2939: const KuduSession* s
> nit: since GetWriteOpMetrics() has become a const method, the style guide d
Done
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/integration-tests/exactly_once_writes-itest.cc
File src/kudu/integration-tests/exactly_once_writes-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/integration-tests/exactly_once_writes-itest.cc@178
PS7, Line 178: esponse.has_resource_metrics()) {
> Just to reassure it's a right thing to do, could you please add a comment e
You are right. The rest of responses actually match.
Added more descriptive comment.
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Apr 2022 17:52:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18451
to look at the new patch set (#3).
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
[client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
KUDU-1563 adds support for INSERT_IGNORE, UPDATE_IGNORE, and
DELETE_IGNORE. However, it is currently lack of per-session metrics
about how many rows get ignored vs modified. This patch implement
per-session metrics as WriteOpMetrics that is sent to client within
WriteResponsePB.
Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/tablet/ops/write_op.cc
M src/kudu/tserver/tserver.proto
8 files changed, 102 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/18451/3
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
......................................................................
Patch Set 4:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@7
PS4, Line 7: in
nit: 'into' or 'to'
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@10
PS4, Line 10: it is currently lack of
nit: it was lacking the ...
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@11
PS4, Line 11: implement
: per-session metrics
implements the per-session metrics
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@12
PS4, Line 12: as ResourceMetricsPB that is sent to client within
: WriteResponsePB
nit: how about
by introducing a new ResourceMetricsPB field into the WriteResponsePB that's populated in every response sent back to the client
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc@973
PS4, Line 973: std::lock_guard<simple_spinlock> l(lock_);
I'm not sure I understand the reason having this guard here. Could you please add a comment to explain why it's needed?
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client-test.cc@2947
PS4, Line 2947: std::map<std::string, int64_t>
nit: this could be 'auto'
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client-test.cc@2948
PS4, Line 2948: ASSERT_EQ
In those asserts, the expected value should be the first argument: that way it's much easier to read the error message if the assertion ever fails.
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.h@2569
PS4, Line 2569: since the session was started
nit: either 'since the session has started' or 'since the beginning of the session'
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.cc@1450
PS4, Line 1450: { return data_->write_op_metrics_; }
nit: please format this method implementation as the rest of the methods in this file -- don't use single line since it's unreadable
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.h
File src/kudu/client/session-internal.h:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.h@168
PS4, Line 168: void UpdateWriteOpMetrics(const tserver::ResourceMetricsPB& metrics);
Please add a doc blurb explaining the essence what this method does.
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.cc@589
PS4, Line 589: const Reflection*
nit for here and below: this could be 'const auto*' for brevity
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tablet/ops/write_op.cc@294
PS4, Line 294: before calling FinishApplyingOrAbort
Could you add a blurb explaining why this is done before calling FinishApplyingOrAbort()?
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tablet/ops/write_op.cc@304
PS4, Line 304: (type() == consensus::LEADER) {
: if (state()->external_consistency_mode() == COMMIT_WAIT) {
nit: could join these two conditionals into one using '&' to have just a single level of if() closures?
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tserver/tserver.proto@189
PS4, Line 189: The resource usage of this RPC.
This looks a bit cryptic to me. Could you please add information what's currently encoded in the 'resource_metrics' field as of now?
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Apr 2022 03:07:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 5:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@7
PS4, Line 7: ResourceMetric
> This should be changed to "ResourceMetricsPB"
Done
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@7
PS4, Line 7: PB
> nit: 'into' or 'to'
Done
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@10
PS4, Line 10: it was lacking the per-
> nit: it was lacking the ...
Done
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@11
PS4, Line 11: the per-session
: metrics by introduc
> implements the per-session metrics
Done
http://gerrit.cloudera.org:8080/#/c/18451/4//COMMIT_MSG@12
PS4, Line 12: ng a new ResourceMetricsPB field into the
: WriteResponsePB
> nit: how about
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc
File src/kudu/client/batcher.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/batcher.cc@973
PS4, Line 973: }
> I'm not sure I understand the reason having this guard here. Could you ple
This follow an example in Batcher::CheckForFinishedFlush() on how to access the session.
But looking again, since we're not inspecting any batcher state here, I think it is safe to remove the simple_spinlock.
This is now removed in patch set 5.
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client-test.cc@2947
PS4, Line 2947: auto metrics = session->GetWri
> nit: this could be 'auto'
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client-test.cc@2948
PS4, Line 2948: ASSERT_EQ
> In those asserts, the expected value should be the first argument: that way
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.h@2569
PS4, Line 2569: since the beginning of the se
> nit: either 'since the session has started' or 'since the beginning of the
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/client.cc@1450
PS4, Line 1450: {
> nit: please format this method implementation as the rest of the methods in
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.h
File src/kudu/client/session-internal.h:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.h@168
PS4, Line 168: // Adds given write operation metrics into session's total write operation metrics.
> Please add a doc blurb explaining the essence what this method does.
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/client/session-internal.cc@589
PS4, Line 589: const auto* refle
> nit for here and below: this could be 'const auto*' for brevity
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tablet/ops/write_op.cc
File src/kudu/tablet/ops/write_op.cc:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tablet/ops/write_op.cc@294
PS4, Line 294: before calling FinishApplyingOrAbort
> Could you add a blurb explaining why this is done before calling FinishAppl
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tablet/ops/write_op.cc@304
PS4, Line 304: p_metrics->set_delete_ignore_errors(op_m.delete_ignore_errors);
: if (type() == consensus::LEADER && state()->external_consist
> nit: could join these two conditionals into one using '&' to have just a si
Done
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:
http://gerrit.cloudera.org:8080/#/c/18451/4/src/kudu/tserver/tserver.proto@189
PS4, Line 189: The write operation metrics of
> This looks a bit cryptic to me. Could you please add information what's cu
Done
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 28 Apr 2022 18:00:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 8: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/client/client-test.cc@2939
PS7, Line 2939: KuduSession* session
nit: since GetWriteOpMetrics() has become a const method, the style guide dictates passing this parameter as a const reference.
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/integration-tests/exactly_once_writes-itest.cc
File src/kudu/integration-tests/exactly_once_writes-itest.cc:
http://gerrit.cloudera.org:8080/#/c/18451/7/src/kudu/integration-tests/exactly_once_writes-itest.cc@178
PS7, Line 178: esponse.has_resource_metrics()) {
Just to reassure it's a right thing to do, could you please add a comment explaining the reasoning behind. I guess the reason is that in case of exactly-once RPC semantics, metrics in retried requests all come zeroed out or even not populated, right?
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Apr 2022 16:42:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18451 )
Change subject: [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
......................................................................
Patch Set 8:
(1 comment)
Patch set 8 fix the IWYU error and submitted to rerun the precommit tests.
I ended up changing exacly_once_writes-itest by dropping the resource metrics from the responses as it is not relevant for the assertion.
http://gerrit.cloudera.org:8080/#/c/18451/6/src/kudu/tablet/ops/write_op.h
File src/kudu/tablet/ops/write_op.h:
http://gerrit.cloudera.org:8080/#/c/18451/6/src/kudu/tablet/ops/write_op.h@264
PS6, Line 264: // Copy metrics from 'op_metrics_' into the response's
> Forgot to write documentation here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/18451
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9adefd64b0058c66274a00e1b12334653fcab2b3
Gerrit-Change-Number: 18451
Gerrit-PatchSet: 8
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 29 Apr 2022 16:09:59 +0000
Gerrit-HasComments: Yes