[kudu-CR] [client] add DISALLOW COPY AND ASSIGN() for a few classes
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18511 ) Change subject: [client] add DISALLOW_COPY_AND_ASSIGN() for a few classes .. Patch Set 1: Code-Review+1 Looks good. I check all 'data_' with comment '// Owned' in client.h and schema.h. With this patch, they all either have copy constructor defined or DISALLOW_COPY_AND_ASSIGN macro declared. I wonder though, can we tell clang-tidy to check these kind of occurrences? -- To view, visit http://gerrit.cloudera.org:8080/18511 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5369760db3040c0357517903dab6ff4e2acb7656 Gerrit-Change-Number: 18511 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Tue, 10 May 2022 16:57:51 + Gerrit-HasComments: No
[kudu-CR] [client] prohibit copying/assigning of ResourceMetrics
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18510 ) Change subject: [client] prohibit copying/assigning of ResourceMetrics .. Patch Set 1: Code-Review+1 Thank you for fixing this issue! The code looks good to me. -- To view, visit http://gerrit.cloudera.org:8080/18510 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I602cc4e194a975752687d13d525e44043955a5cf Gerrit-Change-Number: 18510 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 10 May 2022 05:13:37 + Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18489 ) Change subject: [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API .. Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java File java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java@188 PS1, Line 188: Reso > style nit: the indent should be 4 spaces, IIRC Done http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java@129 PS1, Line 129:*/ > nit: does it make sense to add @Nullable annotation here? Done http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java File java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@35 PS1, Line 35: * A container for scanner resource metrics. : * : * This class wraps a mapping from metric name to metric value for server-side : * metrics associated with a scanner > nit: could you please update this? The ResourceMetrics are now used in the Done http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@87 PS1, Line 87: increment(entry.getKey(), en > nit: this variable is used only once in the increment() call below, so mayb Done http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@104 PS1, Line 104: > nit: PB Done http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java@104 PS1, Line 104: > nit: not be Done http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java: http://gerrit.cloudera.org:8080/#/c/18489/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java@347 PS1, Line 347: private > nit: maybe, change to private if it's not used anywhere else? Done -- To view, visit http://gerrit.cloudera.org:8080/18489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I956eb0c0a2cadcf3491550630b861bb48462e8eb Gerrit-Change-Number: 18489 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 05 May 2022 18:24:38 + Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18489 to look at the new patch set (#2). Change subject: [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API .. [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API The work done in the scope of KUDU-3351 included the server-side changes and corresponding changes in the Kudu C++ API to expose the metrics to the client applications. This patch implement similar changes to expose such metrics in Java client API. Change-Id: I956eb0c0a2cadcf3491550630b861bb48462e8eb --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/BatchResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java M java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java 9 files changed, 120 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/18489/2 -- To view, visit http://gerrit.cloudera.org:8080/18489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I956eb0c0a2cadcf3491550630b861bb48462e8eb Gerrit-Change-Number: 18489 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/18489 ) Change subject: [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API .. Patch Set 1: Hi Alex, this is the follow up patch from KUDU-3351. Please kindly review. -- To view, visit http://gerrit.cloudera.org:8080/18489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I956eb0c0a2cadcf3491550630b861bb48462e8eb Gerrit-Change-Number: 18489 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 04 May 2022 20:52:50 + Gerrit-HasComments: No
[kudu-CR] [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18489 Change subject: [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API .. [client] KUDU-3365: Expose INSERT/UPDATE metrics in the Java API The work done in the scope of KUDU-3351 included the server-side changes and corresponding changes in the Kudu C++ API to expose the metrics to the client applications. This patch implement similar changes to expose such metrics in Java client API. Change-Id: I956eb0c0a2cadcf3491550630b861bb48462e8eb --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/BatchResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/OperationResponse.java M java/kudu-client/src/main/java/org/apache/kudu/client/ResourceMetrics.java M java/kudu-client/src/main/java/org/apache/kudu/client/SessionConfiguration.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduSession.java 9 files changed, 116 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/18489/1 -- To view, visit http://gerrit.cloudera.org:8080/18489 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I956eb0c0a2cadcf3491550630b861bb48462e8eb Gerrit-Change-Number: 18489 Gerrit-PatchSet: 1 Gerrit-Owner: 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 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
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
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
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
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
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
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Thu, 28 Apr 2022 01:58:04 + Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Comment-Date: Thu, 28 Apr 2022 01:54:16 +
Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in 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 (#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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Comment-Date: Wed, 27 Apr 2022 21:49:52 + Gerrit-HasComments: Yes
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in WriteResponsePB
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.c
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in 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 (#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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client] KUDU-3351: Add WriteOpMetrics in 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 (#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 Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120)
[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
[kudu-CR] [rpc] Expose KRPC call id to client application
Riza Suminto has posted comments on this change. (
http://gerrit.cloudera.org:8080/17866 )
Change subject: [rpc] Expose KRPC call_id to client application
..
Patch Set 3:
(12 comments)
Thank you for the feedback!
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1589
PS1, Line 1589: // Set up client.
: shared_ptr client_messenger;
> Consider dropping this once you are done with developing this test scenario
Dropped.
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1595
PS1, Line 1595: for (int i = 0; i < 10; i++) {
: AddRequestPB req;
: req.set_x(rand());
> Is this assert essential for this scenario? If yes, please add a comment e
Dropped.
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1603
PS1, Line 1603: ASSERT_OK(p.SyncReq
> nit: move this closer to the place where 'resp' is used.
Done
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1606
PS1, Line 1606: ASSERT_EQ(i, controller.call_id());
> That's the default setting for credentials policy: consider dropping this l
Dropped.
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1607
PS1, Line 1607:
> Here and below: replace CHECK_XXX() with ASSERT_XXX(): failing the test wit
Done
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1611
PS1, Line 1611: namespace kudu
> Once replaced with ASSERT_EQ(), swap the order of the arguments -- the expe
Done
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.h
File src/kudu/rpc/rpc_context.h:
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.h@230
PS1, Line 230:
> nit: there isn't much sense in returning a constant by value
Done
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.cc
File src/kudu/rpc/rpc_context.cc:
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.cc@146
PS1, Line 146: int32_t RpcContext::call_id() const {
> warning: return type 'const int32_t' (aka 'const int') is 'const'-qualified
Done
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_context.cc@147
PS1, Line 147: call_->call_id();
> Might it be just call->call_id() ?
Done
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.h
File src/kudu/rpc/rpc_controller.h:
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.h@213
PS1, Line 213: he call's Respo
> nit: could you extend this comment to explain what "call's finished" means
Since RpcController reside in the client, "call's finished" means the Response
has been received.
Updated the comment accordingly.
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.cc
File src/kudu/rpc/rpc_controller.cc:
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.cc@106
PS1, Line 106: int32_t RpcController::call_id() const {
> warning: return type 'const int32_t' (aka 'const int') is 'const'-qualified
Done
http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc_controller.cc@106
PS1, Line 106: int32
> nit: returning constant primitive types by value doesn't make much sense --
Done
--
To view, visit http://gerrit.cloudera.org:8080/17866
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb
Gerrit-Change-Number: 17866
Gerrit-PatchSet: 3
Gerrit-Owner: Riza Suminto
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Riza Suminto
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 24 Sep 2021 01:27:14 +
Gerrit-HasComments: Yes
[kudu-CR] [rpc] Expose KRPC call id to client application
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17866 to look at the new patch set (#3). Change subject: [rpc] Expose KRPC call_id to client application .. [rpc] Expose KRPC call_id to client application KRPC assign call_id number for each RPC call at Request header. This call_id then sent back in the Response and allows to match it to the original Request. KRPC client, such as Impala, can utilize this call_id for tracing or logging slow RPC. This patch expose call_id through RpcController and RpcContext class for such purpose. Testing: - Add TestCallId in rpc-test.cc. - Add call_id check in CalculatorService::Add(). - Run and pass rpc-test. Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb --- M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_controller.cc M src/kudu/rpc/rpc_controller.h 6 files changed, 47 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/17866/3 -- To view, visit http://gerrit.cloudera.org:8080/17866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb Gerrit-Change-Number: 17866 Gerrit-PatchSet: 3 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [rpc] Expose KRPC call id to client application
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17866 ) Change subject: [rpc] Expose KRPC call_id to client application .. Patch Set 2: > Patch Set 2: Verified-1 > > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/24507/ : FAILURE patch set 2 fix clang-tidy and IWYU errors. The precommit test failed RaftConsensusITest.TestSlowFollower and TabletHistoryGcITest.TestSnapshotScanBeforeAHM test in DEBUG and ASAN build accordingly. But I don't think they are related with this patch. -- To view, visit http://gerrit.cloudera.org:8080/17866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb Gerrit-Change-Number: 17866 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 23 Sep 2021 21:18:12 + Gerrit-HasComments: No
[kudu-CR] [rpc] Expose KRPC call id to client application
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17866 to look at the new patch set (#2). Change subject: [rpc] Expose KRPC call_id to client application .. [rpc] Expose KRPC call_id to client application KRPC assign call_id number for each RPC call at Request header. This call_id then sent back in the Response and allows to match it to the original Request. KRPC client, such as Impala, can utilize this call_id for tracing or logging slow RPC. This patch expose call_id through RpcController and RpcContext class for such purpose. Testing: - Add TestCallId in rpc-test.cc. - Add call_id check in CalculatorService::Add(). - Run and pass rpc-test. Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb --- M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_controller.cc M src/kudu/rpc/rpc_controller.h 6 files changed, 51 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/17866/2 -- To view, visit http://gerrit.cloudera.org:8080/17866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb Gerrit-Change-Number: 17866 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [rpc] Expose KRPC call id to client application
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17866 ) Change subject: [rpc] Expose KRPC call_id to client application .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: http://gerrit.cloudera.org:8080/#/c/17866/1/src/kudu/rpc/rpc-test.cc@1611 PS1, Line 1611: CHECK_EQ(controller.call_id(), i); > Curious the client assigns the call_id and it's monotonically increasing id AFAIK, call_id is monotonically increasing per host across all RPC https://github.com/apache/kudu/blob/fb4017c/src/kudu/rpc/connection.h#L293-L299 -- To view, visit http://gerrit.cloudera.org:8080/17866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb Gerrit-Change-Number: 17866 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 23 Sep 2021 19:56:11 + Gerrit-HasComments: Yes
[kudu-CR] [rpc] Expose KRPC call id to client application
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17866 Change subject: [rpc] Expose KRPC call_id to client application .. [rpc] Expose KRPC call_id to client application KRPC assign call_id number for each RPC call at Request header. This call_id then sent back in the Response and allows to match it to the original Request. KRPC client, such as Impala, can utilize this call_id for tracing or logging slow RPC. This patch expose call_id through RpcController and RpcContext class for such purpose. Testing: - Add TestCallId in rpc-test.cc. - Add call_id check in CalculatorService::Add(). - Run and pass rpc-test. Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb --- M src/kudu/rpc/rpc-test-base.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_context.cc M src/kudu/rpc/rpc_context.h M src/kudu/rpc/rpc_controller.cc M src/kudu/rpc/rpc_controller.h 6 files changed, 48 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/17866/1 -- To view, visit http://gerrit.cloudera.org:8080/17866 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If20114ef2b416ed9b39277e90639a6277b226fbb Gerrit-Change-Number: 17866 Gerrit-PatchSet: 1 Gerrit-Owner: Riza Suminto
