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 2: (4 comments) 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" Unfortunately, this file isn't among the files included into the set of headers coming with the Kudu C++ client, and it has too many dependencies to be added into that set. I guess you'd need to add a new structure somewhere in the 'exported' header files in src/kudu/client or src/kudu/common (see https://github.com/apache/kudu/blob/50b1cc45f9fde3deac5aa0fef216f4950246b2c9/src/kudu/client/CMakeLists.txt#L184-L213). That new structure would mirror tablet::OpMetrics (but please use 64-bit counters instead of 32-bit ones in tablet::OpMetrics). Probably, src/kudu/client/resource_metrics.h is a good place to add that. Alternatively, you could try to use ResourceMetrics as it's done for scan operations (again, see src/kudu/client/resource_metrics.{h,cc} and its usage throughout the Kudu C++ code, like in https://github.com/apache/kudu/blob/50b1cc45f9fde3deac5aa0fef216f4950246b2c9/src/kudu/client/scanner-internal.cc#L234-L248). http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/client.h@2570 PS2, Line 2570: tablet::OpMetrics GetWriteMetrics() Could this method be 'const' (like the client() method below)? 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: tserver::WriteOpMetricsPB & nit: in Kudu we use google's C++ code style (https://google.github.io/styleguide/cppguide.html), so the asterisk should be stick to the type, not the name of the parameter. http://gerrit.cloudera.org:8080/#/c/18451/2/src/kudu/client/error_collector.h@56 PS2, Line 56: AddWriteMetrics nit: I guess 'IncrementWriteMetrics' or 'UpdateWriteMetrics' might be a better name 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: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 27 Apr 2022 21:31:00 +0000 Gerrit-HasComments: Yes
