Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14652 )
Change subject: KUDU-2162 Expose stats about scan filters ...................................................................... Patch Set 2: (4 comments) I haven't reviewed the client-side stuff yet. http://gerrit.cloudera.org:8080/#/c/14652/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/14652/1/src/kudu/tserver/tablet_service.cc@1611 PS1, Line 1611: Nit: don't need this extra space. http://gerrit.cloudera.org:8080/#/c/14652/1/src/kudu/tserver/tablet_service.cc@1622 PS1, Line 1622: MonoTime now = MonoTime::Now(); : MonoDelta duration; : if (scan_descriptor.state == ScanState::kActive) { : duration = now - scan_descriptor.start_time; : } else { : duration = scan_descriptor.last_access_time - scan_descriptor.start_time; : } This was copied from tserver_path_handlers.cc; could you decompose it into a ScanDescriptor helper method? Something like: pair<MonoDelta, MonoDelta> GetScanDurationAndTimeSinceStart() const; http://gerrit.cloudera.org:8080/#/c/14652/2/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/14652/2/src/kudu/tserver/tablet_service.cc@1613 PS2, Line 1613: const ScanDescriptor& scan_descriptor) { There are some short-circuit paths that return OK but don't populate the ScanDescriptor, like L2503 for example. How will this function behave if given a default-constructed ScanDescriptor? I think the duration calculation may be messed up. What about the rest? http://gerrit.cloudera.org:8080/#/c/14652/2/src/kudu/tserver/tablet_service.cc@2692 PS2, Line 2692: *scan_descriptor = scanner.get()->descriptor(); This makes a copy of the ScanDescriptor with each Scan RPC. That's somewhat expensive given the contents of the ScanDescriptor (e.g. projected_columns, predicates, iterator_stats). Given that we only want some quantity metrics (timings and bytes read), perhaps we can avoid the copy? Some ideas: 1. Pass the result collector into the Scanner so that the Scanner can write the metrics into the collector, then use the collector to set up the ResourceMetricsPB when the RPC finishes. 2. Decompose the metrics you care about into a new struct that ScanDescriptor wraps, then get that struct from the Scanner here instead of the full ScanDescriptor. -- To view, visit http://gerrit.cloudera.org:8080/14652 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I127b96a30467ea0abcaf6ed5b4d72df66b2dec55 Gerrit-Change-Number: 14652 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 07 Nov 2019 22:59:13 +0000 Gerrit-HasComments: Yes
