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 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/9680/12/be/src/runtime/io/request-context.cc File be/src/runtime/io/request-context.cc: http://gerrit.cloudera.org:8080/#/c/9680/12/be/src/runtime/io/request-context.cc@74 PS12, Line 74: if (outcome == ReadOutcome::SUCCESS_EOSR) { : // No more reads to do. : --disk_state->num_remaining_ranges(); : } else if (outcome == ReadOutcome::SUCCESS_NO_EOSR) { : // Schedule the next read. : if (state_ != RequestContext::Cancelled) { : ScheduleScanRange(lock, range); : } : } else if (outcome == ReadOutcome::BLOCKED_ON_BUFFER) { : // Do nothing - the caller must add a buffer to the range or cancel it. : } else { : DCHECK(outcome == ReadOutcome::CANCELLED) << static_cast<int>(outcome); : // No more reads - clean up the scan range. : --disk_state->num_remaining_ranges(); : RemoveActiveScanRangeLocked(lock, range); : } > I tried both earlier and thought this was actually more readable (it was cl FWIW I find the if-stmt easier to read also, though don't feel too strongly either. Also, not that it matters in this case, but this structure implies (and optimizes for) the fast and slow paths, which can be nice. -- 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: 12 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: Wed, 28 Mar 2018 17:09:23 +0000 Gerrit-HasComments: Yes
