Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10810 )
Change subject: IMPALA-7095: clean up scan node profiles ...................................................................... Patch Set 5: (6 comments) Did an initial pass over the code. Found some minor issues. http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/hdfs-scan-node-base.h@489 PS5, Line 489: per_read_thread I guess it's rather per_thread_read_throughput , just like in its comment. http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.h File be/src/exec/scan-node.h: http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.h@46 PS5, Line 46: layet Nit: layer http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.h@95 PS5, Line 95: Nit: too much spaces http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.h@234 PS5, Line 234: && Nit: I think it's not necessary here and it would be nice if it was consistent with EnqueueBatch(). http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.h@256 PS5, Line 256: && Since this function does not always take ownership of 'row_batch', I think it is confusing to see std::move at the call sites. Maybe a non-const l-value reference or a pointer would be a little bit more straightforward. http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.cc@143 PS5, Line 143: /// Starts counters that should start measuring in the Open() phase. : void StartOpenCounters(); It seems it is some leftover code. -- To view, visit http://gerrit.cloudera.org:8080/10810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77286282d42e7764bfdf94c7ec47cec9d743f787 Gerrit-Change-Number: 10810 Gerrit-PatchSet: 5 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 27 Jun 2018 16:02:10 +0000 Gerrit-HasComments: Yes
