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 7: (10 comments) http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.h File be/src/runtime/io/scan-buffer-manager.h: http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.h@136 PS7, Line 136: void SetCachedBuffer() { nit. may inline. http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.h@140 PS7, Line 140: void SetClientBuffer() { same as above. http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.h@144 PS7, Line 144: void SetNoBuffer() { same as above 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@169 PS4, Line 169: iomgr_buffer_cu > That's the name from the existing code and it still is a tag used for exter Done 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@199 PS4, Line 199: << " cancel_status: > Thats a precondition i.e., callers of PopReadyBuffer (like ScanRange::GetNe Done http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.cc File be/src/runtime/io/scan-buffer-manager.cc: http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.cc@28 PS7, Line 28: // Implementation of the buffer management for ScanRange. Each ScanRange contains a queue : // of ready buffers. For each ScanRange, there is only a single producer and consumer : // thread, i.e. only one disk thread will push to a scan range at any time and only one : // thread will remove from the queue. This is to guarantee that buffers are queued and : // read in file order. nit. This para describes the class very well. Maybe move it to the .h file? http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.cc@138 PS7, Line 138: DCHECK(scan_range_->is_locked(scan_range_lock)); Passing the scan_range_lock and calling DCHECK() is the price we pay for the refactoring. Just wonder if scan_range_lock can be removed entirely. That is, we assume the lock is acquired. The exception is the CloseReader() call below takes the lock. I was not able to find the implementation of the method. Maybe the lock argument is not needed? http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.cc@186 PS7, Line 186: for (auto& buffer : unused_iomgr_buffers_) nit. Should add '{' here because the statement below is not at the same line. 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@427 PS4, Line 427: int disk_id, bool expected_local, int64_t mtime, const BufferOpts& buffer_opts, : vector<SubRange>&& sub_ranges, void* meta_data, DiskFile > This is an assignment not a check. I guess you meant to add a Setter which Done http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-range.cc@432 PS4, Line 432: HECK(file != nullptr); : DCHECK_GE(len, 0); > This is an assignment not a check. I guess you meant to add a Setter which Done -- 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: 7 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 26 May 2021 15:06:52 +0000 Gerrit-HasComments: Yes
