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 <[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 18:00:30 +0000 Gerrit-HasComments: Yes
