Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8375 )
Change subject: KUDU-2162 Expose stats about scan filters ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/8375/4/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/8375/4/src/kudu/client/client-test.cc@457 PS4, Line 457: ASSERT_GT(metrics["cpu_system_nanos"], 0); > I am not sure why this test is failing on jenkins. I added non pk column in It's difficult to get timing tests to pass 100% of the time. Maybe the ContainsKey() checks are good enough? Or just drop the ASSERT_GT stuff where timings are involved? http://gerrit.cloudera.org:8080/#/c/8375/4/src/kudu/rpc/inbound_call.h File src/kudu/rpc/inbound_call.h: http://gerrit.cloudera.org:8080/#/c/8375/4/src/kudu/rpc/inbound_call.h@71 PS4, Line 71: MonoDelta WaitDuration() const { This can be used in RecordHandlingCompleted. http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/rpc/rpc_context.h File src/kudu/rpc/rpc_context.h: http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/rpc/rpc_context.h@217 PS5, Line 217: // Return the inbound call timing Nit: terminate with a period. http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/rpc/rpc_context.h@218 PS5, Line 218: const InboundCallTiming& inboundCallTiming() const; Nit: InboundCallTiming(). Or inbound_call_timing(). http://gerrit.cloudera.org:8080/#/c/8375/4/src/kudu/tserver/scanners.h File src/kudu/tserver/scanners.h: http://gerrit.cloudera.org:8080/#/c/8375/4/src/kudu/tserver/scanners.h@408 PS4, Line 408: // ScanRPCMetrics contain metrics that will be sent to the client for each batch of rows Seems like we could remove this and just use CpuTimes directly now? http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/tserver/tablet_service.cc@1753 PS5, Line 1753: rpc::InboundCallTiming timing = context->inboundCallTiming(); : timing.time_completed = MonoTime::Now(); I see that you're making a copy of the InboundCallTiming (so there's no danger of the inbound call machinery setting time_completed a second time), but it still looks a little weird. If you pass 'context' in its entirety into SetResourceMetrics, you can extract the raw MonoTimes from it and use them to calculate wait_duration/total_duration, obviating the need to modify the InboundCallTiming instance. http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/tserver/tablet_service.cc@1755 PS5, Line 1755: collector.SetResourceMetrics(context->trace(), timing, resp->mutable_resource_metrics()); Let's pass 'context' into the function so we don't need to duplicate the timing stuff in both SetResourceMetrics calls. http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/tserver/tserver.proto File src/kudu/tserver/tserver.proto: http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/tserver/tserver.proto@373 PS5, Line 373: optional int64 wait_duration_nanos = 4; Let's call this queue_duration_nanos to be more clear about the state of the RPC during this time (it was sitting in a queue). -- To view, visit http://gerrit.cloudera.org:8080/8375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id30a7e82357fe2fc28f6d316378a612af43d8c96 Gerrit-Change-Number: 8375 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 12 Nov 2019 07:03:29 +0000 Gerrit-HasComments: Yes
