Tim Armstrong 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: (6 comments) LMK if there's somethign that could be clarified about the error propagation and cancellation (either in the code or by talking out-of-band). We definitely want it to be as clear as possible. 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 buffe Good point, I mechanically converted a lot of the code in here, but in this place it isn't needed. 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 Done http://gerrit.cloudera.org:8080/#/c/8707/18/be/src/runtime/io/disk-io-mgr.h@271 PS18, Line 271: AddBuffersToRange > AllocateBuffersForRange? Done 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. Done 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 wou Thanks for the idea, I went ahead and did it. I also changed the name of the function to "AddRangeToDisk()". That seems to convey what it is actually doing a little better, but let me know what you think. 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_ Unfortunately not exactly, the meaning here is essentially "in DiskIoMgr" whereas DiskIoMgr has a specific internal concept of "in flight". -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 29 Jan 2018 05:30:34 +0000 Gerrit-HasComments: Yes
