Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8814 )
Change subject: IMPALA-6290: limit ScannerContext to 1 buffer at a time ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/8814/9/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: http://gerrit.cloudera.org:8080/#/c/8814/9/be/src/exec/hdfs-scanner.cc@a195 PS9, Line 195: : : This is not needed because we no longer accumulate completed IO buffers as there is only one outstanding IO buffer outstanding per stream, right ? http://gerrit.cloudera.org:8080/#/c/8814/9/be/src/exec/scanner-context.h File be/src/exec/scanner-context.h: http://gerrit.cloudera.org:8080/#/c/8814/9/be/src/exec/scanner-context.h@196 PS9, Line 196: /// Release completed resources, e.g. the last buffer if the current read position is nit: one line. http://gerrit.cloudera.org:8080/#/c/8814/9/be/src/exec/scanner-context.h@305 PS9, Line 305: /// nit: io_buffer_ is set to null. http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: http://gerrit.cloudera.org:8080/#/c/8814/6/be/src/exec/scanner-context.cc@239 PS6, Line 239: if (boundary_buffer_bytes_left_ >= requested_len) break; > CopyIoToBoundary() copies over some or all of the I/O buffer increments bou I am still not entirely convinced. Upon entry of the loop body, the invariant is that boundary_buffer_bytes_left_ + io_buffer_bytes_left_ < requested_len_. So, even after copying the entire I/O buffer over, boundary_buffer_bytes_left_ should still be less than requested_len_. So, copying part of the I/O buffer will definitely not make buffer_bytes_left_ >= requested_len_. I could be missing something but this if-condition seems to be always false and that's what confuses me. -- To view, visit http://gerrit.cloudera.org:8080/8814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74c5960a75f7d88b0e1de4199af731fb13e592f0 Gerrit-Change-Number: 8814 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Wed, 10 Jan 2018 19:57:40 +0000 Gerrit-HasComments: Yes
