Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10550 )
Change subject: IMPALA-7078: Part 1: improve memory consumption of wide Avro scans ...................................................................... Patch Set 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/10550/11/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/10550/11/be/src/exec/hdfs-scan-node.cc@186 PS11, Line 186: AVERAGE_SCANNER_THREAD_CONCURRENCY, &active_scanner_thread_counter_); > would it be better to move this above line 183 so that other code doesn't h It's so the average time doesn't include the time when the scan node is unopened. E.g. if the scan is opened late in the query lifecycle (maybe it's on the right side of a join or something), we don't want to include all the time beforehand when no scanner threads are expected to be running. http://gerrit.cloudera.org:8080/#/c/10550/11/be/src/exec/hdfs-scan-node.cc@192 PS11, Line 192: > nit: Intentional newline? Not really but I move the code anyway in part 2 of the patch, so I won't bother changing it. http://gerrit.cloudera.org:8080/#/c/10550/11/be/src/exec/scan-node.h File be/src/exec/scan-node.h: http://gerrit.cloudera.org:8080/#/c/10550/11/be/src/exec/scan-node.h@191 PS11, Line 191: /// Cumulative number of scanner threads created during the scan. Some may be created : /// and then destroyed, so this can exceed the peak number of threads. > This is a great observability addition to this counter. But to someone who Honestly, I think this counter isn't very useful, but I didn't necessarily want to remove it as part of this change. I figured we should at least document it. -- To view, visit http://gerrit.cloudera.org:8080/10550 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iebd2600b4784fd19696c9b92eefb7d7e9db0c80b Gerrit-Change-Number: 10550 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vincent Tran <[email protected]> Gerrit-Comment-Date: Wed, 06 Jun 2018 20:55:19 +0000 Gerrit-HasComments: Yes
