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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 27 Apr 2022 04:30:41 +0000 Gerrit-HasComments: Yes
