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 10: (21 comments) http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG@20 PS9, Line 20: Testing: : 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. : 2. Ran the above tes > this has been outdated Done http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG@24 PS9, Line 24: Change- > We talked about it offline, but for the record, please run exhaustive and t Ran exhaustive + TSAN and it passed: https://master-03.jenkins.cloudera.com/job/impala-private-parameterized/220/ http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/request-context.cc File be/src/runtime/io/request-context.cc: http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/request-context.cc@545 PS9, Line 545: // ** 'range->buffer_manager_->buffer_tag' is CACHED_BUFFER and the buffer is : // already available to the reader. : // (there is nothing to do) : // > There is no 'buffer_tag' anymore, please update comment. Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/request-ranges.h File be/src/runtime/io/request-ranges.h: http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/request-ranges.h@306 PS9, Line 306: bool i > A function defined within a class definition is implicitly inline, no need Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/request-ranges.h@346 PS9, Line 346: bool i > Same as above, implicitly inline already. Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h File be/src/runtime/io/scan-buffer-manager.h: http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@38 PS9, Line 38: // > nit: please add an empty line above "BUFFER LIFECYCLE" to make the text a b Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@42 PS9, Line 42: eturn > nit: buffers? Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@56 PS9, Line 56: cache. Ag > nit, optional: I wonder if it would make sense to rename it to IO_MGR_BUFFE I have renamed it to INTERNAL_BUFFER and removed external from the ExternalBufferTag. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@65 PS9, Line 65: > This is not a polymorphic class, so we don't need to make the dtor virtual. Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@75 PS9, Line 75: to be a > nit: at least Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@126 PS9, Line 126: > nit: we should use std::string in headers Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@129 PS9, Line 129: ss << " buffer_queue=" << ready_buffers_.size() > nit: please add +4 indent. But I'm good with +3 as well, so the first '<<'s Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@135 PS9, Line 135: > nit: use ' instead of ` Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@144 PS9, Line 144: /// ScanRange us > Should we hold the lock for doing the validation? yes it is needed actually but it was only being used for DCHECK. Have added it now. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@146 PS9, Line 146: /// th > nit: no need for the inline keyword. Please take care of the below function Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@186 PS9, Line 186: : /// Scan range that uses t > nit: unnecessary comment Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@219 PS9, Line 219: /// that can be allocated. Additionally, cumulative sum of buffer sizes should not > nit: please add a short comment Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.cc File be/src/runtime/io/scan-buffer-manager.cc: http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.cc@29 PS9, Line 29: buffer_tag_(BufferTag::INTERNAL_BUFFER) { > nit: unnecessary empty line Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-range.cc File be/src/runtime/io/scan-range.cc: http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-range.cc@95 PS9, Line 95: buffer_manager_->add_buffers_in_reader(1); > Can we move this to PopFirstReadyBuffer()? By moving it to PopFirstReadyBuffer(), we will end up forcing the caller to be GetNext() only. It can possibly be used in other places in future like CleanupReadyBuffers etc where this counter should not change. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-range.cc@122 PS9, Line 122: if (unblocked) ScheduleScanRange(); : } : > nit: fits single line Done http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-range.cc@153 PS9, Line 153: ad_in_flight_ = true; > Can we move this to GetUnusedBuffer()? This is also used by CleanupUnusedBuffers where above counter is not relevant. -- 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: 10 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: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 01 Jun 2021 23:40:06 +0000 Gerrit-HasComments: Yes
