[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17413 ) Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr .. IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr Currently BufferManagement is tightly coupled with ScanRange. Every ScanRange maintains list of unused buffers and ready buffers. Unused buffers are buffers used to read scanned data and ready buffers are buffers with the data already read. For managing these buffers, ScanRange defines various functions like AddUnusedBuffer, GetUsedBuffer, EnqueueReadyBuffer and functions to allocate and cleanup buffers. This patch has created ScanBufferManager which would be responsible for the managing these buffers for ScanRange. ScanBufferManager's logic is still coupled with the ScanRange, but refactorig it into a seperate class is a good first step. Testing: 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. 2. Ran the above tests with TSAN build using exhaustive strategy. Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Reviewed-on: http://gerrit.cloudera.org:8080/17413 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h A be/src/runtime/io/scan-buffer-manager.cc A be/src/runtime/io/scan-buffer-manager.h M be/src/runtime/io/scan-range.cc 7 files changed, 609 insertions(+), 244 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- 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: merged Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 12 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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 11: Verified+1 -- 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: 11 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 03 Jun 2021 16:14:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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 11: Code-Review+2 -- 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: 11 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 03 Jun 2021 10:22:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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 11: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7195/ DRY_RUN=false -- 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: 11 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 03 Jun 2021 10:22:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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 10: Code-Review+2 (1 comment) Thanks for doing this refactoring, I think the code is much cleaner now! http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG@24 PS9, Line 24: Change- > Private exhaustive tests passed too: https://master-03.jenkins.cloudera.com Thanks for running all these tests! -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 03 Jun 2021 10:21:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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: (1 comment) http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG@24 PS9, Line 24: Change- > Just exhaustive test pass: https://jenkins.impala.io/job/pre-review-test/96 Private exhaustive tests passed too: https://master-03.jenkins.cloudera.com/job/impala-private-parameterized/233/ -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 02 Jun 2021 12:03:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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: (1 comment) http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17413/9//COMMIT_MSG@24 PS9, Line 24: Change- > Ran exhaustive + TSAN and it passed: https://master-03.jenkins.cloudera.com Just exhaustive test pass: https://jenkins.impala.io/job/pre-review-test/967/ -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 02 Jun 2021 10:05:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8827/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 02 Jun 2021 00:01:12 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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: > Glad to see someone new getting their hands dirty in this code, it > makes sense to make ScanRange responsible for fewer things... Thanks Tim. -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Tue, 01 Jun 2021 23:40:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Amogh Margoor has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/17413 ) Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr .. IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr Currently BufferManagement is tightly coupled with ScanRange. Every ScanRange maintains list of unused buffers and ready buffers. Unused buffers are buffers used to read scanned data and ready buffers are buffers with the data already read. For managing these buffers, ScanRange defines various functions like AddUnusedBuffer, GetUsedBuffer, EnqueueReadyBuffer and functions to allocate and cleanup buffers. This patch has created ScanBufferManager which would be responsible for the managing these buffers for ScanRange. ScanBufferManager's logic is still coupled with the ScanRange, but refactorig it into a seperate class is a good first step. Testing: 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. 2. Ran the above tests with TSAN build using exhaustive strategy. Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h A be/src/runtime/io/scan-buffer-manager.cc A be/src/runtime/io/scan-buffer-manager.h M be/src/runtime/io/scan-range.cc 7 files changed, 609 insertions(+), 244 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/17413/10 -- 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: newpatchset Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 10 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Tim Armstrong 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: Glad to see someone new getting their hands dirty in this code, it makes sense to make ScanRange responsible for fewer things... -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Sat, 29 May 2021 06:18:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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 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:
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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 9: Code-Review+1 (1 comment) Looks good! 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@138 PS7, Line 138: // Close the reader if there are no buffer in th > Even in earlier code many of these functions were already passing scan_rang 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: 9 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 27 May 2021 13:09:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8806/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 27 May 2021 08:41:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Amogh Margoor has uploaded a new patch set (#9). ( http://gerrit.cloudera.org:8080/17413 ) Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr .. IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr Currently BufferManagement is tightly coupled with ScanRange. Every ScanRange maintains list of unused buffers and ready buffers. Unused buffers are buffers used to read scanned data and ready buffers are buffers with the data already read. For managing these buffers, ScanRange defines various functions like AddUnusedBuffer, GetUsedBuffer, EnqueueReadyBuffer and functions to allocate and cleanup buffers. This patch has created ScanBufferManager which would be responsible for the managing these buffers for ScanRange. ScanBufferManager's logic is still coupled with the ScanRange, but refactorig it into a seperate class is a good first step. As ScanRange's lock is used to synchronize various functions including the buffer management, a new class for lock store has been created too. Testing: 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h A be/src/runtime/io/scan-buffer-manager.cc A be/src/runtime/io/scan-buffer-manager.h M be/src/runtime/io/scan-range.cc 7 files changed, 593 insertions(+), 230 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/17413/9 -- 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: newpatchset Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 9 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8805/ : Initial code review checks failed. See linked job for details on the failure. -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Thu, 27 May 2021 08:03:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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* 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
Gerrit-Reviewer: Amogh Margoor
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Thu, 27 May 2021 07:54:31 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Amogh Margoor has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/17413 ) Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr .. IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr Currently BufferManagement is tightly coupled with ScanRange. Every ScanRange maintains list of unused buffers and ready buffers. Unused buffers are buffers used to read scanned data and ready buffers are buffers with the data already read. For managing these buffers, ScanRange defines various functions like AddUnusedBuffer, GetUsedBuffer, EnqueueReadyBuffer and functions to allocate and cleanup buffers. This patch has created ScanBufferManager which would be responsible for the managing these buffers for ScanRange. ScanBufferManager's logic is still coupled with the ScanRange, but refactorig it into a seperate class is a good first step. As ScanRange's lock is used to synchronize various functions including the buffer management, a new class for lock store has been created too. Testing: 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h A be/src/runtime/io/scan-buffer-manager.cc A be/src/runtime/io/scan-buffer-manager.h M be/src/runtime/io/scan-range.cc 7 files changed, 593 insertions(+), 230 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/17413/8 -- 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: newpatchset Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 8 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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&& 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
Gerrit-Reviewer: Amogh Margoor
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Qifan Chen
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Wed, 26 May 2021 15:06:52 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8796/ : Initial code review checks failed. See linked job for details on the failure. -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 26 May 2021 13:51:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Amogh Margoor has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/17413 ) Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr .. IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr Currently BufferManagement is tightly coupled with ScanRange. Every ScanRange maintains list of unused buffers and ready buffers. Unused buffers are buffers used to read scanned data and ready buffers are buffers with the data already read. For managing these buffers, ScanRange defines various functions like AddUnusedBuffer, GetUsedBuffer, EnqueueReadyBuffer and functions to allocate and cleanup buffers. This patch has created ScanBufferManager which would be responsible for the managing these buffers for ScanRange. ScanBufferManager's logic is still coupled with the ScanRange, but refactorig it into a seperate class is a good first step. As ScanRange's lock is used to synchronize various functions including the buffer management, a new class for lock store has been created too. Testing: 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h A be/src/runtime/io/scan-buffer-manager.cc A be/src/runtime/io/scan-buffer-manager.h M be/src/runtime/io/scan-range.cc 7 files changed, 584 insertions(+), 230 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/17413/7 -- 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: newpatchset Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 7 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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 5: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8795/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 5 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 26 May 2021 11:30:27 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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 5:
(25 comments)
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@250
PS4, Line 250: class ScanRange : public RequestRan
> We need to specify what fields it protects in ScanRange. Or, we should stil
Removed this class.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@252
PS4, Line 252:
> nit. needs
Code deleted.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@514
PS4, Line 514:
> +1 on "ensuring no new locks are taken in ScanBufferManager methods.
This is done now. No locks acquired in ScanBufferManager.
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@36
PS4, Line 36: ange using this buffer manager.
> Actually there isn't much info at 'external_buffer_tag_'. Could you add com
Have updated the documentation.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@40
PS4, Line 40: ///This buffer is external to this buffer manager and is
not managed by it
> nit. missing '.'.
Done
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@47
PS4, Line 47:
> nit. nullptr.
Done
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@52
PS4, Line 52:
> nit. should be 'scan_range_'.
Done
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@53
PS4, Line 53:
> nit. this scan range (i.e. scan_range_).
Done
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@62
PS4, Line 62:
> nit. scan_range_->len().
Done
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@106
PS4, Line 106: scan_range_loc
> nit. PopFirstReadyBuffer() describes the semantics better.
Done
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@109
PS4, Line 109: /// Clean
> nit. should be inline.
Done
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@131
PS4, Line 131: /// ScanRange using thi
> Instead of the friend declaration, can we add public functions that ScanRan
Have removed friend from both the classes. Neither ScanRange can access private
methods/members of ScanBufferManager nor the other way is possible now. This
also prevents ScanBufferManager taking ScanRange's lock_.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@136
PS4, Line 136: void SetCachedBuffer() {
: external_buffer_tag_ = ExternalBuffe
> I think having this pointer is a bit confusing, probably it'd be a bit clea
This code is deleted now as discussed.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@169
PS4, Line 169: iomgr_buffer_cu
> nit: maybe name it BufferTag, since the buffer is managed by this new class
That's the name from the existing code and it still is a tag used for external
buffers. External buffers are tagged as CLIENT_BUFFER and CACHED_BUFFER,
whereas managed buffers NO_BUFFER (to depict that it is not an external buffer).
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@177
PS4, Line 177:
> nit: add +2 indentation
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@28
PS4, Line 28: mplementation of the buffer managem
> This probably should be moved inside the cstr body, after DCHECK().
Code is deleted now.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@85
PS4, Line 85: nused_iomgr_buffer_bytes_ -= result->buffer_le
> please update comment
Done.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@135
PS4, Line 135: ::CleanUpBu
> nit. can we replace goto with the actual code below?
done.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@162
PS4, Line 162: Buff
> this->scan_range?
done.
http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@165
PS4, Line 165: return false;
: }
: *buffer = move(ready_buffers_.front());
: ready_buffers_.pop_front();
: // If eosr is seen, then unused buffer should be empty.
: DCHECK(!(*buffer)->eosr() ||
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/17413/5/be/src/runtime/io/scan-buffer-manager.cc File be/src/runtime/io/scan-buffer-manager.cc: http://gerrit.cloudera.org:8080/#/c/17413/5/be/src/runtime/io/scan-buffer-manager.cc@89 PS5, Line 89: void ScanBufferManager::EnqueueReadyBuffer(const std::unique_lock& scan_range_lock, line too long (95 > 90) -- 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: 5 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 26 May 2021 11:11:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Amogh Margoor has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/17413 ) Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr .. IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr Currently BufferManagement is tightly coupled with ScanRange. Every ScanRange maintains list of unused buffers and ready buffers. Unused buffers are buffers used to read scanned data and ready buffers are buffers with the data already read. For managing these buffers, ScanRange defines various functions like AddUnusedBuffer, GetUsedBuffer, EnqueueReadyBuffer and functions to allocate and cleanup buffers. This patch has created ScanBufferManager which would be responsible for the managing these buffers for ScanRange. ScanBufferManager's logic is still coupled with the ScanRange, but refactorig it into a seperate class is a good first step. As ScanRange's lock is used to synchronize various functions including the buffer management, a new class for lock store has been created too. Testing: 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-ranges.h A be/src/runtime/io/scan-buffer-manager.cc A be/src/runtime/io/scan-buffer-manager.h M be/src/runtime/io/scan-range.cc 7 files changed, 585 insertions(+), 231 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/17413/5 -- 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: newpatchset Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 5 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Qifan Chen Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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 4: (1 comment) 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@514 PS4, Line 514: lock_store_ > Actually the logic in ScanBufferManager and ScanRange are still coupled and EnqueueReadyBuffer() and AddUnusedBuffer() acquire the lock at the beginning, so it wouldn't make harm to pre-acquire the locks. But CleanUpBuffers() also acquires the lock, and it is invoked by AllocateBuffersForRange() at the error path. Taking a look again AddUnusedBuffer() is also invoked by AllocateBuffersForRange() in the middle of the method. One way to resolve this is to put AllocateBuffersForRange() back to ScanRange and acquiring the lock there, so AddUnusedBuffer() and CleanUpBuffers() and all other member functions in ScanBufferManager can expect a pre-acquired lock. I think I like this idea as it'll probably make the reasoning clearer about locking. -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 19 May 2021 16:48:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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 4: (1 comment) 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@514 PS4, Line 514: lock_store_ > Now this lock protects the members of the scan range, and the members of th Actually the logic in ScanBufferManager and ScanRange are still coupled and both modify each others member variables currently. Additionally there are parts in ScanBufferManager which needs ScanRange lock to be freed before scheduling scan range due to existing ordering between locks of ScanRange and RequestContext. So with this coupling it would be cleaner to have only one lock (which we can keep in ScanRange as suggested instead of moving to new class). However, we can make semantics cleaner by ensuring no new locks are taken in ScanBufferManager methods. ScanRange's lock is pre-acquired (if needed) before invoking the buffer manager's methods. All existing method follows that except EnqueueReadyBuffer and AddUnusedBuffer which I can refactor and ensure it follows that. Would that make things better ? -- 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 Gerrit-Reviewer: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 19 May 2021 15:46:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
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 4: (8 comments) Did an initial round. Overall looks good, will do another rounds next week. 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@250 PS4, Line 250: /// Lock protecting fields below. We need to specify what fields it protects in ScanRange. Or, we should still keep the 'lock_' there. Since there's only one lock here we probably don't need this class. Maybe we could just add a public Lock() method to ScanRange. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/request-ranges.h@514 PS4, Line 514: lock_store_ Now this lock protects the members of the scan range, and the members of the ScanRangeBuffer as well, right? Probably it'd be clearer if we had a two locks, one in scan range, one in scan range buffer, and define a clear ordering for how they should be acquired when we need to lock both. 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@36 PS4, Line 36: See external_buffer_tag_ for details. Actually there isn't much info at 'external_buffer_tag_'. Could you add comment that describes the semantics for each value? http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@131 PS4, Line 131: friend class ScanRange; Instead of the friend declaration, can we add public functions that ScanRange could use? http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@136 PS4, Line 136: /// Scan range lock store which are needed for accessing buffer. : ScanRangeLockStore* const lock_store_; I think having this pointer is a bit confusing, probably it'd be a bit cleaner to use the lock via 'scan_range_'. I think we should either have 2 separate locks for ScanRange and ScanRangeBuffer, or only keep the lock in ScanRange and remove this pointer. http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.h@177 PS4, Line 177: st nit: add +2 indentation 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@85 PS4, Line 85: Implementation of the ScanRange functionality. please update comment http://gerrit.cloudera.org:8080/#/c/17413/4/be/src/runtime/io/scan-buffer-manager.cc@162 PS4, Line 162: this this->scan_range? -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Fri, 14 May 2021 16:05:07 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8703/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Wed, 12 May 2021 10:25:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr
Amogh Margoor has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/17413 ) Change subject: IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr .. IMPALA-7556: Decouple BufferManagement from the ScanRange and IoMgr Currently BufferManagement is tightly coupled with ScanRange. Every ScanRange maintains list of unused buffers and ready buffers. Unused buffers are buffers used to read scanned data and ready buffers are buffers with the data already read. For managing these buffers, ScanRange defines various functions like AddUnusedBuffer, GetUsedBuffer, EnqueueReadyBuffer and functions to allocate and cleanup buffers. This patch has created ScanBufferManager which would be responsible for the managing these buffers for ScanRange. ScanBufferManager's logic is still coupled with the ScanRange, but refactorig it into a seperate class is a good first step. As ScanRange's lock is used to synchronize various functions including the buffer management, a new class for lock store has been created too. Testing: 1. Ran these existing tests: EE, BackEnd, JDBC and Cluster test. Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/request-context.cc M be/src/runtime/io/request-context.h M be/src/runtime/io/request-ranges.h A be/src/runtime/io/scan-buffer-manager.cc A be/src/runtime/io/scan-buffer-manager.h M be/src/runtime/io/scan-range.cc 8 files changed, 531 insertions(+), 282 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/13/17413/4 -- 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: newpatchset Gerrit-Change-Id: Ibd74691b50b46114f95a8641034c05d07ddeec97 Gerrit-Change-Number: 17413 Gerrit-PatchSet: 4 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tamas Mate Gerrit-Reviewer: Zoltan Borok-Nagy
