Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9680 )
Change subject: IMPALA-6587: free buffers before ScanRange::Cancel() returns ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/disk-io-mgr.cc@667 PS8, Line 667: } consider getting rid of this subroutine, it doesn't seem to add to readability. http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/request-context.h File be/src/runtime/io/request-context.h: http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/request-context.h@381 PS8, Line 381: Dequeue what is the "dequeue" portion of this name referring to? http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/request-ranges.h@475 PS8, Line 475: /// Number of bytes of buffers returned from GetNextUnusedBufferForRange(). Used to : /// infer how many bytes of buffers need to be held onto to read the rest of the scan : /// range. : int64_t iomgr_buffer_bytes_returned_ = 0; comment needs updating, and not sure if the counter name still makes sense or could be improved now? http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc File be/src/runtime/io/scan-range.cc: http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc@192 PS8, Line 192: iomgr_buffer_bytes_returned_ += buffer_desc->buffer_len(); hard to understand what this is about from the field's name. returned to where? http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc@201 PS8, Line 201: // Update counters. seems superfluous http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc@274 PS8, Line 274: , ? http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc@279 PS8, Line 279: void ScanRange::ReadFailed(const Status& status, unique_ptr<BufferDescriptor> buffer) { : DCHECK(!status.ok()); : // Free buffer to release resources before we cancel the range so that all buffers : // are freed at cancellation. : reader_->FreeBuffer(buffer.get()); : buffer.reset(); : : CancelInternal(status, true); : } not sure this subroutine really helps rather than just doing this work in DoRead(). http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc@314 PS8, Line 314: if (read_error) { : DCHECK(read_in_flight_); : read_in_flight_ = false; : } this is a bit confusing. It's not intuitive why CancelInternal() would need to do this for the caller, but I guess it has to do with the locking? -- To view, visit http://gerrit.cloudera.org:8080/9680 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I87182b6bd51b5fb0b923e7e4c8d08a44e7617db2 Gerrit-Change-Number: 9680 Gerrit-PatchSet: 8 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 19 Mar 2018 23:53:05 +0000 Gerrit-HasComments: Yes
