Tim Armstrong 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: (9 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 readabil Done 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? This is called when dequeueing the disk state from the disk queue. I renamed to IncrementDiskThreadAfterDequeue() and updated the comment to make that clearer. 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 Renamed to iomgr_buffer_cumulative_bytes_used_, which I think is more precise but still not elegant. Didn't have a better idea. 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@63 PS8, Line 63: // One or more threads may be blocked in CancelInternal() waiting for the read to This comment was incorrect, its actually in WaitForInFlightRead() now. 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 wh Renamed - see header comment. http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc@201 PS8, Line 201: // Update counters. > seems superfluous Done http://gerrit.cloudera.org:8080/#/c/9680/8/be/src/runtime/io/scan-range.cc@274 PS8, Line 274: , > ? Should have been a period. 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 D That's true, removed it. 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 Yeah, since we need to acquire 'lock_' in this function anyway. It also means that waking up threads in WaitForInFlightRead() is a post-condition of this function. I documented that post-condition - maybe that makes it a bit clearer? -- 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: Tue, 20 Mar 2018 21:23:10 +0000 Gerrit-HasComments: Yes
