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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 28 Apr 2022 03:07:33 +0000 Gerrit-HasComments: Yes
