Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8414 )
Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt ...................................................................... Patch Set 4: (3 comments) Testing found one subtle bug. http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr-reader-context.cc File be/src/runtime/disk-io-mgr-reader-context.cc: http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr-reader-context.cc@50 PS4, Line 50: DCHECK(buffer->scan_range()->external_buffer_tag_ == ScanRange::ExternalBufferTag::NO_BUFFER) > long line Done http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr.cc@462 PS4, Line 462: // Buffers the were not allocated by DiskIoMgr don't need to be freed. > existing typo: that Done http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr.cc@797 PS4, Line 797: reader->disk_states_[disk_queue->disk_id].DecrementRequestThread(); There were a subtle bug here that could cause readers to get wedged. The problem is that we should call DecrementRequestThreadAndCheckDone() when the range is cancelled, since it's possible that the RequestContext was cancelled in the meantime and this is the last thread that needs to do the cleanup. Ideally we would rethink the names of some of these functions and clarify the protocol but I don't want to get too distracted from the main point of this work. -- To view, visit http://gerrit.cloudera.org:8080/8414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1 Gerrit-Change-Number: 8414 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 06 Nov 2017 17:37:55 +0000 Gerrit-HasComments: Yes
