Zoltan Borok-Nagy 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 9: (21 comments) The change looks really great, thank you! I've had a couple of comments, but most of them just nits. 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: As ScanRange's lock is used to synchronize various functions : including the buffer management, a new class for lock store : has been created too. this has been outdated http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG@24 PS9, Line 24: Testing We talked about it offline, but for the record, please run exhaustive and tsan tests. 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: // ** buffer_tag is CACHED_BUFFER and the buffer is already available to the reader. : // (there is nothing to do) : // : // * The scan range has sub-ranges, and buffer_tag is: There is no 'buffer_tag' anymore, please update comment. 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: inline A function defined within a class definition is implicitly inline, no need for the keyword. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/request-ranges.h@346 PS9, Line 346: inline Same as above, implicitly inline already. 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: // BUFFER LIFECYCLE: nit: please add an empty line above "BUFFER LIFECYCLE" to make the text a bit less dense http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@42 PS9, Line 42: buffer nit: buffers? http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@56 PS9, Line 56: NO_BUFFER nit, optional: I wonder if it would make sense to rename it to IO_MGR_BUFFER, or MANAGED_BUFFER, or DYNAMIC_BUFFER, or stg. like that. If so, then we could rename ExternalBufferTag to BufferTag. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@65 PS9, Line 65: virtual This is not a polymorphic class, so we don't need to make the dtor virtual. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@75 PS9, Line 75: atleast nit: at least http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@126 PS9, Line 126: string nit: we should use std::string in headers side-note: seems like we transitively include "common/names.h" unfortunately, otherwise it shouldn't compile. We should never include that in headers, but this is a different issue. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@129 PS9, Line 129: << " num_buffers_in_readers=" << num_buffers_in_reader_.Load() nit: please add +4 indent. But I'm good with +3 as well, so the first '<<'s are aligned http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@135 PS9, Line 135: `*buffer` nit: use ' instead of ` http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@144 PS9, Line 144: bool Validate(); Should we hold the lock for doing the validation? http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@146 PS9, Line 146: inline nit: no need for the inline keyword. Please take care of the below functions as well. http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@186 PS9, Line 186: ///////////////////////////////////////// : /// BEGIN: private members nit: unnecessary comment http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-buffer-manager.h@219 PS9, Line 219: static std::vector<int64_t> ChooseBufferSizes(int64_t scan_range_len, nit: please add a short comment 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: nit: unnecessary empty line 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()? 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 http://gerrit.cloudera.org:8080/#/c/17413/9/be/src/runtime/io/scan-range.cc@153 PS9, Line 153: buffer_manager_->add_iomgr_buffer_cumulative_bytes_used(buffer_desc->buffer_len()); Can we move this to GetUnusedBuffer()? -- 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: 9 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 15:44:43 +0000 Gerrit-HasComments: Yes
