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 3: (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: AddWriteMetrics > Isn't this overwriting the metrics instead of merging them? From the descr You're right, this should sum rather than override. Fixed in patch set 2. 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 sen Done 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 Done http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2567 PS1, Line 2567: /// Get the total of write operation metrics during the lifetime of a session. > Should this method be marked as constant one? Fixed in patch set 3. http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/client/client.h@2567 PS1, Line 2567: /// Ge > nit: is it possible to drop the 'kudu::' part given the outer namespace is Done 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: > Why not to use constant reference for the parameter? The argument isn't mo Done 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 particu Yes, we want to accumulate per-session metrics on the client side. Does the change in patch set 2 (ErrorCollector::AddWriteMetrics) address this concern? On different issue, I can't seem to run client-test manually myself. I always get this error: I0427 14:43:01.165669 21595 catalog_manager.cc:1375] Loading table and tablet metadata into memory... I0427 14:43:01.166294 21595 catalog_manager.cc:1384] Initializing Kudu cluster ID... *** Aborted at 1651095781 (unix time) try "date -d @1651095781" if you are using GNU date *** PC: @ 0x7febcb8f87f3 kudu::tserver::WriteResponsePB::_internal_mutable_metrics() *** SIGSEGV (@0x10) received by PID 21364 (TID 0x7feb98f9b700) from PID 16; stack trace: *** @ 0x7febc9359f71 google::(anonymous namespace)::FailureSignalHandler() @ 0x7febca7ca980 (unknown) @ 0x7febcb8f87f3 kudu::tserver::WriteResponsePB::_internal_mutable_metrics() @ 0x7febcb8f884e kudu::tserver::WriteResponsePB::mutable_metrics() @ 0x7febcb8f48cc kudu::tablet::WriteOp::Finish() @ 0x7febcb8e31c4 kudu::tablet::OpDriver::Finalize() @ 0x7febcb8e2bf1 kudu::tablet::OpDriver::ApplyTask() @ 0x7febcb8e1b51 _ZZN4kudu6tablet8OpDriver10ApplyAsyncEvENKUlvE_clEv @ 0x7febcb8e490f _ZNSt17_Function_handlerIFvvEZN4kudu6tablet8OpDriver10ApplyAsyncEvEUlvE_E9_M_invokeERKSt9_Any_data @ 0x7febccc00796 std::function<>::operator()() @ 0x7febca07bc18 kudu::ThreadPool::DispatchThread() @ 0x7febca07c501 _ZZN4kudu10ThreadPool12CreateThreadEvENKUlvE_clEv @ 0x7febca07dc15 _ZNSt17_Function_handlerIFvvEZN4kudu10ThreadPool12CreateThreadEvEUlvE_E9_M_invokeERKSt9_Any_data @ 0x7febccc00796 std::function<>::operator()() @ 0x7febca06d36b kudu::Thread::SuperviseThread() @ 0x7febca7bf6db start_thread @ 0x7febc7fb661f clone [1] 21364 segmentation fault ./bin/client-test Is there any mistakes in how I initialize WriteOpMetrics here? 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. Done http://gerrit.cloudera.org:8080/#/c/18451/1/src/kudu/tserver/tserver.proto@167 PS1, Line 167: optional uint64 successful_inserts = 1; : optional uint64 insert_ignore_errors = 2; : optional uint64 successful_upserts = 3; : optional uint64 successful_updates = 4; : optional uint64 update_ignore_errors = 5; : optional uint64 successful_deletes = 6; : optional uint64 delete_ignore_errors = 7; : optional uint64 commit_wait_duration_usec = 8; > I guess all these should be optional, especially in the context of eventual 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: 3 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: Wed, 27 Apr 2022 21:46:34 +0000 Gerrit-HasComments: Yes
