Tim Armstrong 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) 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. The name of the counter is "PerReadThreadRawHdfsThroughput", which isn't a great name, but the names of the variables reflect the counter name. Not sure if it's worth changing the counter name - I didn't have a great alternative. 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 Done http://gerrit.cloudera.org:8080/#/c/10810/5/be/src/exec/scan-node.h@95 PS5, Line 95: > Nit: too much spaces Done 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 consist Done 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 Yeah we had an extended discussion about this pattern at one point: https://gerrit.cloudera.org/#/c/6527/5/be/src/exec/kudu-scan-node.cc@179 I'd find lvalue references confusing since then there's no indication that ownership might be transferred at the callsite. I switched this to just pass in a pointer to the unique_ptr, since that seems to avoid both potential confusions. I think the pattern is harder to avoid in BlockingQueue itself, since it's generic and meant to work for copyable value types too. 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. Done -- 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: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 28 Jun 2018 00:41:57 +0000 Gerrit-HasComments: Yes
