[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Sat, 30 Apr 2022 20:07:01 + Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 --- 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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Sat, 30 Apr 2022 05:00:49 + Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Fri, 29 Apr 2022 21:00:51 +
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Fri, 29 Apr 2022 17:52:56 +
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Fri, 29 Apr 2022 16:42:58 +
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 29 Apr 2022 16:09:59 + Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 #include +#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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Fri, 29 Apr 2022 16:01:04 + Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Thu, 28 Apr 2022 23:14:32 +
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 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 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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Thu, 28 Apr 2022 21:34:12 +
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Thu, 28 Apr 2022 18:00:30 +
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add ResourceMetricsPB into WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
