Dan Hecht 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 19: (10 comments) Next set of comments. Remaining TODO: parquet related and tests. http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h File be/src/exec/scanner-context.h: http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@222 PS19, Line 222: when reading past the end of 'scan_range_' what about the reservation for reading the scan_range_? why does this "read past" reservation need to be treated special? http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@223 PS19, Line 223: I/O buffer : /// size is that talking about the "read size" or "BP min buffer size"? http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@284 PS19, Line 284: int64_t reservation, is this the "read past" reservation or something else? http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.h@372 PS19, Line 372: /// this context. this answers my question about constructor. Maybe reference this function from that constructor comment. http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: http://gerrit.cloudera.org:8080/#/c/8966/19/be/src/exec/scanner-context.cc@153 PS19, Line 153: The buffers for 'scan_range_' should be released now : // since we hit EOS. if that's true, why can't we reuse their reservation? Or maybe we actually are reusing the reservation, but the header comments tricked me into thinking we had a separate reservation set aside? http://gerrit.cloudera.org:8080/#/c/8966/18/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/8966/18/be/src/runtime/io/request-ranges.h@65 PS18, Line 65: be accounted against 'mem_tracker update. http://gerrit.cloudera.org:8080/#/c/8966/19/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/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@155 PS19, Line 155: MIN_SCAN_RANGE_RESERVATION can't we delete this now? http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@186 PS19, Line 186: time a time? http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1330 PS19, Line 1330: required to ensure the scan : * can succeed while avoiding excessively tiny I/O operations. isn't this more about the min buffer size? http://gerrit.cloudera.org:8080/#/c/8966/19/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1345 PS19, Line 1345: backend iomgrScanRangesPerFile. Actually, the PerFile isn't right either since multiple scanner threads can be working on the same file (different splits). So how about iomgrScanRangesPerSplit? -- 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: 19 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: Wed, 21 Feb 2018 01:02:28 +0000 Gerrit-HasComments: Yes
