[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/8375 )
Change subject: KUDU-2162 Expose stats about scan filters ...................................................................... Patch Set 7: (8 comments) > Patch Set 6: > > (9 comments) Fixed. There is only a single test failure that appears unrelated. DenseNodeTest.RunTest test_workload.cc:306] Timed out: Timed out waiting for Table Creation 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: } > It's difficult to get timing tests to pass 100% of the time. Maybe the Cont Done 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 ProcessingDuration() const { > This can be used in RecordHandlingCompleted. I had a bug in this method, it should return (time_handled - time_received). I fixed it and created a new method ProcessingDuration that returns (time_completed - time_handled) and used it 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: MonoTime GetTimeHandled() const; > Nit: terminate with a period. Done http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/rpc/rpc_context.h@218 PS5, Line 218: > Nit: InboundCallTiming(). Or inbound_call_timing(). Done 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: // ScanDescriptor holds information about a scan. The ScanDescriptor can outlive > Seems like we could remove this and just use CpuTimes directly now? Done 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: } : resp->set_propagated_timestamp(server_-> > I see that you're making a copy of the InboundCallTiming (so there's no dan Done http://gerrit.cloudera.org:8080/#/c/8375/5/src/kudu/tserver/tablet_service.cc@1755 PS5, Line 1755: > Let's pass 'context' into the function so we don't need to duplicate the ti Done 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 queue_duration_nanos = 4; > Let's call this queue_duration_nanos to be more clear about the state of th Done -- 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: 7 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 21:09:53 +0000 Gerrit-HasComments: Yes
