Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/17413 )
Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr ...................................................................... Patch Set 4: (18 comments) Looks good and I may go over it one more time this week. Overall, I like the idea of decoupling ScanRange and the new class and maybe we can achieve a good level of separation. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@252 PS4, Line 252: need nit. needs http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@514 PS4, Line 514: lock_store_ > EnqueueReadyBuffer() and AddUnusedBuffer() acquire the lock at the beginnin +1 on "ensuring no new locks are taken in ScanBufferManager methods. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h File be/src/runtime/io/scan-buffer-manager.h: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@40 PS4, Line 40: /// management for the respective range nit. missing '.'. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@47 PS4, Line 47: NULL nit. nullptr. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@52 PS4, Line 52: range nit. should be 'scan_range_'. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@53 PS4, Line 53: the scan range nit. this scan range (i.e. scan_range_). http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@62 PS4, Line 62: range nit. scan_range_->len(). http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@106 PS4, Line 106: PopReadyBuffer nit. PopFirstReadyBuffer() describes the semantics better. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@109 PS4, Line 109: ExternalB nit. should be inline. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@169 PS4, Line 169: ExternalBufferTag nit: maybe name it BufferTag, since the buffer is managed by this new class and is not external. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc File be/src/runtime/io/scan-buffer-manager.cc: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@28 PS4, Line 28: lock_store_(&(range->lock_store_)), This probably should be moved inside the cstr body, after DCHECK(). http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@135 PS4, Line 135: goto error; nit. can we replace goto with the actual code below? http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@165 PS4, Line 165: if (scan_range_->all_buffers_returned(scan_range_lock) && : num_buffers_in_reader_.Load() == 0) { : // Close the scan range if there are no more buffers in the reader and no more buffers : // will be returned to readers in future. Close() is idempotent so it is ok to call : // multiple times during cleanup so long as the range is actually finished. : if (!scan_range_->use_local_buffer_) { : scan_range_->file_reader_->Close(); : } else { : scan_range_->local_buffer_reader_->Close(); : } It looks like this block of code is scan_range_ specific and probably should be moved back to the ScanRange as a method like CloseReader(). We can call it here. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@199 PS4, Line 199: DCHECK(!ready_buffers_.empty()); Should return false if the buffer is empty as DCHECK() only works in debug build. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc File be/src/runtime/io/scan-range.cc: http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@88 PS4, Line 88: external_buffer_tag_ nit. This data member does not exist anymore. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@427 PS4, Line 427: buffer_manager_->external_buffer_tag_ = : ScanBufferManager::ExternalBufferTag::CLIENT_BUFFER; nit. Call buffer_manager_->is_client_buffer() instead? http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@432 PS4, Line 432: buffer_manager_->external_buffer_tag_ = : ScanBufferManager::ExternalBufferTag::NO_BUFFER; Same comment as above. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@553 PS4, Line 553: buffer_manager_->external_buffer_tag_ = : ScanBufferManager::ExternalBufferTag::CACHED_BUFFER; nit. May create a set method buffer_manager_ and call it SetCachedBuffer(true)? The idea is to hide the buffer type (as indicated by external_buffer_tag_) inside the new buffer manager. -- To view, visit http://gerrit.cloudera.org:8080/17413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 4 Gerrit-Owner: Amogh Margoor <amarg...@gmail.com> Gerrit-Reviewer: Amogh Margoor <amarg...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 19 May 2021 17:24:44 +0000 Gerrit-HasComments: Yes
