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 <[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 01:54:16 +0000 Gerrit-HasComments: Yes
