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 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/9680/5/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9680/5/be/src/runtime/io/disk-io-mgr.cc@713 PS5, Line 713: Will remove extra blank line. http://gerrit.cloudera.org:8080/#/c/9680/5/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/9680/5/be/src/runtime/io/request-ranges.h@473 PS5, Line 473: bool read_in_flight_ = false; These three boolean flags are all mutually exclusive. I thought about using a status enum and making the state transitions explicit, but it doesn't work out super-cleanly, at least without more invasive changes, since some logical states are implied by the presence of the scan range in one or another queue. We could update the scan range statuses when moving between queues, but that might require acquiring ScanRange::lock_ unnecessarily. Open to suggestions here. I think the logical states of the scan range are something like: NOT_STARTED SCHEDULED READ_IN_FLIGHT BLOCKED_ON_BUFFER EOSR_QUEUED CANCEL_FINISHED The difference between NOT_STARTED, SCHEDULED and DONE is implied by its position in different queues and not necessary to represent. -- 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: 5 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 16 Mar 2018 16:24:07 +0000 Gerrit-HasComments: Yes
