Amogh Margoor 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 8: (6 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: /// Returns 'false', if 'ready_buffer_' is empty and '*buffer' cannot be assigned, > nit. may inline. Done http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.h@140 PS7, Line 140: std::unique_ptr<BufferDescriptor>* buffer); > same as above. Done http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.h@144 PS7, Line 144: bool Validate(); > same as above 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. : : ScanBufferManager::ScanBufferManager(ScanRange* range): scan_range_(range), : external_buffer_tag_(ExternalBufferTag::NO_BUFFER) { : DCHECK(range != null > nit. This para describes the class very well. Maybe move it to the .h file? Done http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.cc@138 PS7, Line 138: // Close the reader if there are no buffer in th > Passing the scan_range_lock and calling DCHECK() is the price we pay for th Even in earlier code many of these functions were already passing scan_range_lock and doing the DCHECK(GetUnusedBuffer, CleanUpBuffer, CleanUpBuffers) and that is just moved to new class. IMO, it is a good practise to do it because the cost of accidentally calling these functions without taking lock can be high - data races can be tricky to figure out. Debug assertion and function signature goes a long way to enforce it in any new call sites and they would not incur too much cost on production code as DCHECKs are disabled. Regarding CloseReader - implementation is in scan_range.cc. It needs lock as it checks the Buffer state and Scan Range state before closing the reader. http://gerrit.cloudera.org:8080/#/c/17413/7/be/src/runtime/io/scan-buffer-manager.cc@186 PS7, Line 186: LOG(ERROR) << "unused_iomgr_buffer_bytes_ incorrect actual: " > nit. Should add '{' here because the statement below is not at the same lin 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: 8 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: Thu, 27 May 2021 07:54:31 +0000 Gerrit-HasComments: Yes
