Bikramjeet Vig 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: Code-Review+1 (4 comments) Looks good, just a few minor things http://gerrit.cloudera.org:8080/#/c/9680/12/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9680/12/be/src/runtime/io/disk-io-mgr.cc@a651 PS12, Line 651: any reason why we needed reader->Validate() at both the beginning and end of this method? Seems like you kept the one at the end in your change which makes sense but just want to be sure we are not missing something by removing this at the beginning. 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); : } looks like a good candidate for a switch-case http://gerrit.cloudera.org:8080/#/c/9680/12/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/9680/12/be/src/runtime/io/request-ranges.h@277 PS12, Line 277: nit: extra space http://gerrit.cloudera.org:8080/#/c/9680/12/be/src/runtime/io/request-ranges.h@477 PS12, Line 477: StartRead() and set to false in EnqueueReadyBuffer() or ReadFailed() update comment, StartRead and ReadFailed dont exist anymore -- 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: Tue, 27 Mar 2018 23:50:57 +0000 Gerrit-HasComments: Yes
