Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8966 )
Change subject: IMPALA-4835: Part 3: switch I/O buffers to buffer pool ...................................................................... Patch Set 17: (5 comments) http://gerrit.cloudera.org:8080/#/c/8966/17/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/8966/17/be/src/exec/hdfs-scan-node-base.h@325 PS17, Line 325: scan range > here we aren't talking about "backend" aka "iomgr" scan ranges, right? May Done. Also updated a similar comments in thrift and the frontend. I thought about renaming to "ideal_input_split_reservation" but that then seemed inconsistent with other terminology. http://gerrit.cloudera.org:8080/#/c/8966/17/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/8966/17/be/src/exec/hdfs-scan-node-base.cc@246 PS17, Line 246: // indicate a misconfiguration or bug. > maybe reference the frontend code that was suppose to adhere to this. Done http://gerrit.cloudera.org:8080/#/c/8966/17/be/src/exec/hdfs-scan-node-base.cc@344 PS17, Line 344: ideal_scan_range_reservation_ - resource_profile_.min_reservation)); > should we at least log the return value for debugging, or is it easy enough You could infer it from the profile and plan by comparing the actual reservation with the minimum, but that is not really reasonable to expect anyone to do. It seems like a reasonable idea to log it. I added a log message at a verbose log level. http://gerrit.cloudera.org:8080/#/c/8966/17/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/8966/17/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1350 PS17, Line 1350: long minReservationBytes = Math.max( : backendScanRangesPerFile * MIN_SCAN_RANGE_RESERVATION, : Math.min(backendScanRangesPerFile * maxIoBufferSize, : quantizedMaxScanRangeBytes)); > I think calling it MIN_IOMGR_SCAN_RANGE_RESERVATION will help clarify the p Switched to using --min_buffer_size. http://gerrit.cloudera.org:8080/#/c/8966/18/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/8966/18/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@155 PS18, Line 155: l_; > Or should it be MIN_IOMGR_SCAN_RANGE_BUFFER_SIZE? But hopefully you can ju Yeah, I'm just getting it from the flag now. We could have some problems still if Impala was configured with different --min_buffer_size values per daemon, but I don't think that is something that we need to support. -- To view, visit http://gerrit.cloudera.org:8080/8966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic09c6196b31e55b301df45cc56d0b72cfece6786 Gerrit-Change-Number: 8966 Gerrit-PatchSet: 17 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 20 Feb 2018 23:20:47 +0000 Gerrit-HasComments: Yes
