[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10493 to look at the new patch set (#2). Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ IMPALA-110 will involve refactoring PartitionedAggregationNode by separating out the aggregation logic into a new type called Aggregator, and then supporting multiple Aggregators per node to allow for multiple aggregation classes to be evaluated at the same time. Each Aggregator will need to have its own memory reservation to operate, and we can do this by giving each Aggregator its own BufferPool::ClientHandle instead of using the usual ExecNode::buffer_pool_client_. To facilitate this, this patch refactors all of the buffer_pool_client_ related logic into a new class, ReservationManager, so that eventually each Aggregator can have its own ReservationManager and the logic in ClaimBufferReservation(), ReleaseUnusedReservation(), etc. won't be duplicated. Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a --- M be/src/exec/analytic-eval-node.cc M be/src/exec/exec-node.cc M be/src/exec/exec-node.h M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/partial-sort-node.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/sort-node.cc M be/src/runtime/CMakeLists.txt A be/src/runtime/reservation-manager.cc A be/src/runtime/reservation-manager.h 13 files changed, 284 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/10493/2 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 2: Code-Review+2 Thanks. I think this makes more sense than having them all as members directly in ExecNode. -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 15 Jun 2018 23:29:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h File be/src/runtime/reservation-manager.h: http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@33 PS2, Line 33: which with? http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@37 PS2, Line 37: static void Register(ReservationManager* rm, std::string name, please document that http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@39 PS2, Line 39: TBackendResourceProfile resource_profile, const TDebugOptions you probably meant to use references here. http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.cc File be/src/runtime/reservation-manager.cc: http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.cc@31 PS2, Line 31: Register this looks more like an Init() function (and doesn't have to be static given the first parameters is effectively 'this'). -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 15 Jun 2018 23:47:04 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 15 Jun 2018 23:58:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Hello Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10493 to look at the new patch set (#3). Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ IMPALA-110 will involve refactoring PartitionedAggregationNode by separating out the aggregation logic into a new type called Aggregator, and then supporting multiple Aggregators per node to allow for multiple aggregation classes to be evaluated at the same time. Each Aggregator will need to have its own memory reservation to operate, and we can do this by giving each Aggregator its own BufferPool::ClientHandle instead of using the usual ExecNode::buffer_pool_client_. To facilitate this, this patch refactors all of the buffer_pool_client_ related logic into a new class, ReservationManager, so that eventually each Aggregator can have its own ReservationManager and the logic in ClaimBufferReservation(), ReleaseUnusedReservation(), etc. won't be duplicated. Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a --- M be/src/exec/analytic-eval-node.cc M be/src/exec/exec-node.cc M be/src/exec/exec-node.h M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/partial-sort-node.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/sort-node.cc M be/src/runtime/CMakeLists.txt A be/src/runtime/reservation-manager.cc A be/src/runtime/reservation-manager.h 13 files changed, 288 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/10493/3 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/10493 )
Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_
..
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h
File be/src/runtime/reservation-manager.h:
http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@33
PS2, Line 33: with
> with?
Done
http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@37
PS2, Line 37: ReservationManager() {}
> please document that
Done
http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.h@39
PS2, Line 39: lize this ReservationManager with the given values. 'name' is
> you probably meant to use references here.
Done
http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.cc
File be/src/runtime/reservation-manager.cc:
http://gerrit.cloudera.org:8080/#/c/10493/2/be/src/runtime/reservation-manager.cc@31
PS2, Line 31: Init(str
> this looks more like an Init() function (and doesn't have to be static give
Done
--
To view, visit http://gerrit.cloudera.org:8080/10493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a
Gerrit-Change-Number: 10493
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Mon, 18 Jun 2018 18:38:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Hello Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10493 to look at the new patch set (#4). Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ IMPALA-110 will involve refactoring PartitionedAggregationNode by separating out the aggregation logic into a new type called Aggregator, and then supporting multiple Aggregators per node to allow for multiple aggregation classes to be evaluated at the same time. Each Aggregator will need to have its own memory reservation to operate, and we can do this by giving each Aggregator its own BufferPool::ClientHandle instead of using the usual ExecNode::buffer_pool_client_. To facilitate this, this patch refactors all of the buffer_pool_client_ related logic into a new class, ReservationManager, so that eventually each Aggregator can have its own ReservationManager and the logic in ClaimBufferReservation(), ReleaseUnusedReservation(), etc. won't be duplicated. Testing: - Passed a full run of the core tests. Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a --- M be/src/exec/analytic-eval-node.cc M be/src/exec/exec-node.cc M be/src/exec/exec-node.h M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/partial-sort-node.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/sort-node.cc M be/src/runtime/CMakeLists.txt A be/src/runtime/reservation-manager.cc A be/src/runtime/reservation-manager.h 13 files changed, 288 insertions(+), 164 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/10493/4 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 Jun 2018 21:05:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 Jun 2018 21:30:55 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 5: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2696/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 Jun 2018 21:30:56 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ IMPALA-110 will involve refactoring PartitionedAggregationNode by separating out the aggregation logic into a new type called Aggregator, and then supporting multiple Aggregators per node to allow for multiple aggregation classes to be evaluated at the same time. Each Aggregator will need to have its own memory reservation to operate, and we can do this by giving each Aggregator its own BufferPool::ClientHandle instead of using the usual ExecNode::buffer_pool_client_. To facilitate this, this patch refactors all of the buffer_pool_client_ related logic into a new class, ReservationManager, so that eventually each Aggregator can have its own ReservationManager and the logic in ClaimBufferReservation(), ReleaseUnusedReservation(), etc. won't be duplicated. Testing: - Passed a full run of the core tests. Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Reviewed-on: http://gerrit.cloudera.org:8080/10493 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/exec/analytic-eval-node.cc M be/src/exec/exec-node.cc M be/src/exec/exec-node.h M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node.cc M be/src/exec/partial-sort-node.cc M be/src/exec/partitioned-aggregation-node.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/sort-node.cc M be/src/runtime/CMakeLists.txt A be/src/runtime/reservation-manager.cc A be/src/runtime/reservation-manager.h 13 files changed, 288 insertions(+), 164 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-110 (part 1): Refactor ExecNode::buffer pool client
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10493 ) Change subject: IMPALA-110 (part 1): Refactor ExecNode::buffer_pool_client_ .. Patch Set 5: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75f92c3f4f05adeef11a70f59e0c8ff2d19bc17a Gerrit-Change-Number: 10493 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 19 Jun 2018 00:49:27 + Gerrit-HasComments: No
