Tianyi Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/8707 )
Change subject: IMPALA-4835: Part 2: Allocate scan range buffers upfront ...................................................................... Patch Set 18: (8 comments) I'm still trying to understand the error propagation and cancellation part. Here are some comments and questions. http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr-test.cc File be/src/runtime/io/disk-io-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr-test.cc@1068 PS18, Line 1068: if (needs_buffers) { Doesn't BufferOpts::ReadInto(client_buffer.data(), SCAN_LEN) provides buffer already? http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.h@147 PS18, Line 147: An extra space http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.h@271 PS18, Line 271: AddBuffersToRange AllocateBuffersForRange? http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.h@271 PS18, Line 271: /// If 'needs_buffers' is set to true, the caller must then call AddBuffersToRange() to Can we merge "the caller must then call..." work into this function and remove this contract? http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.cc@366 PS18, Line 366: Status DiskIoMgr::AddScanRange(RequestContext* reader, ScanRange* range) { This function is unused. http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/request-context.h@205 PS18, Line 205: RequestRange* range, bool schedule_later, bool schedule_immediately); I think combining schedule_later and schedule_immediately into one enum would be more clear. e.g. enum { SCHEDULE_NOW, SCHEDULE_UPON_GETNEXT, SCHEDULE_BY_CALLER } http://gerrit.cloudera.org:8080/#/c/8707/17/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/8707/17/be/src/runtime/io/request-ranges.h@309 PS17, Line 309: that extra 'that' http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/tmp-file-mgr.cc@558 PS18, Line 558: // The write will not be in flight if we returned with an error. Does the in_flight here have the same meaning as RequestContext::in_flight_ranges? -- To view, visit http://gerrit.cloudera.org:8080/8707 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia243bf8b62feeea602b122e0503ea4ba7d3ee70f Gerrit-Change-Number: 8707 Gerrit-PatchSet: 18 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 25 Jan 2018 00:05:55 +0000 Gerrit-HasComments: Yes
