You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org> on 2020/02/28 18:07:35 UTC
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Bikramjeet Vig has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15318
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink
configs where they were created
Currently the exprs created in plan nodes and sink configs were being
closed in their respective exec nodes and data sink instances (that
were created using them) when the fragment instance closes. This patch
ensure that they are closed from the plan nodes and sink configs
themselves so that when these are shared among instances, they don't
prematurely close the expressions.
Testing:
Successfully passed exhaustive tests.
Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
---
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
41 files changed, 230 insertions(+), 147 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/15318/1
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 4: Code-Review+2
rebased and carrying forward Tim and Csaba's +2
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 00:40:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 2: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.h@73
PS2, Line 73: /// Close() releases all resources that were allocated in Init(). Currently it only
The second sentence is probably at risk of getting stale, maybe remove it?
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.cc@91
PS2, Line 91: for (int i = 0; i < children_.size(); ++i) {
Consider using ranged for, just for consistency?
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 29 Feb 2020 01:56:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5394/ : 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/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 01:23:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink
configs where they were created
Currently the exprs created in plan nodes and sink configs were being
closed in their respective exec nodes and data sink instances (that
were created using them) when the fragment instance closes. This patch
ensure that they are closed from the plan nodes and sink configs
themselves so that when these are shared among instances, they don't
prematurely close the expressions.
Testing:
Successfully passed exhaustive tests.
Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Reviewed-on: http://gerrit.cloudera.org:8080/15318
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
41 files changed, 225 insertions(+), 139 deletions(-)
Approvals:
Bikramjeet Vig: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15318
to look at the new patch set (#4).
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink
configs where they were created
Currently the exprs created in plan nodes and sink configs were being
closed in their respective exec nodes and data sink instances (that
were created using them) when the fragment instance closes. This patch
ensure that they are closed from the plan nodes and sink configs
themselves so that when these are shared among instances, they don't
prematurely close the expressions.
Testing:
Successfully passed exhaustive tests.
Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
---
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
41 files changed, 225 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/15318/4
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15318
to look at the new patch set (#3).
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink
configs where they were created
Currently the exprs created in plan nodes and sink configs were being
closed in their respective exec nodes and data sink instances (that
were created using them) when the fragment instance closes. This patch
ensure that they are closed from the plan nodes and sink configs
themselves so that when these are shared among instances, they don't
prematurely close the expressions.
Testing:
Successfully passed exhaustive tests.
Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
---
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
41 files changed, 224 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/15318/3
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5365/ : 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/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Feb 2020 18:52:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 05:37:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5438/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 00:40:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5366/ : 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/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Feb 2020 19:15:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/aggregation-node-base.cc
File be/src/exec/aggregation-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/aggregation-node-base.cc@50
PS2, Line 50: void AggregationPlanNode::Close() {
: for (AggregatorConfig* config : aggs_) config->Close();
: PlanNode::Close();
: }
I don't want to hold this up, just wanted to mention that the whole Expr/PlanNode closing logic looks unnecessary to me: if I didn't miss something, then the only resources that need closing are 'cache_entry_'s that are used by ScalarFnCall to get UDF/UDA files. These could be registered to the fragment instance when we get the file and released during closing.
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 02 Mar 2020 16:24:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/aggregation-node-base.cc
File be/src/exec/aggregation-node-base.cc:
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/aggregation-node-base.cc@50
PS2, Line 50: void AggregationPlanNode::Close() {
: for (AggregatorConfig* config : aggs_) config->Close();
: PlanNode::Close();
: }
> I don't want to hold this up, just wanted to mention that the whole Expr/Pl
I agree, that will save us the effort of adding the close methods, but will have to investigate more to figure out if decoupling of cache_entry_ management and exprs is cleaner and intuitive. will add a JIRA for this:IMPALA-9454
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.h@73
PS2, Line 73: /// Close() releases all resources that were allocated in Init().
> The second sentence is probably at risk of getting stale, maybe remove it?
Done. Here and in data-sink.h
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/15318/2/be/src/exec/exec-node.cc@91
PS2, Line 91: for (auto& child : children_) {
> Consider using ranged for, just for consistency?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 00:30:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15318
to look at the new patch set (#2).
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink
configs where they were created
Currently the exprs created in plan nodes and sink configs were being
closed in their respective exec nodes and data sink instances (that
were created using them) when the fragment instance closes. This patch
ensure that they are closed from the plan nodes and sink configs
themselves so that when these are shared among instances, they don't
prematurely close the expressions.
Testing:
Successfully passed exhaustive tests.
Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
---
M be/src/exec/aggregation-node-base.cc
M be/src/exec/aggregation-node-base.h
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/analytic-eval-node.cc
M be/src/exec/analytic-eval-node.h
M be/src/exec/data-sink.cc
M be/src/exec/data-sink.h
M be/src/exec/exchange-node.cc
M be/src/exec/exchange-node.h
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/nested-loop-join-node.cc
M be/src/exec/nested-loop-join-node.h
M be/src/exec/non-grouping-aggregator.h
M be/src/exec/partial-sort-node.cc
M be/src/exec/partial-sort-node.h
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M be/src/exec/scan-node.cc
M be/src/exec/sort-node.cc
M be/src/exec/sort-node.h
M be/src/exec/topn-node.cc
M be/src/exec/topn-node.h
M be/src/exec/union-node.cc
M be/src/exec/union-node.h
M be/src/exec/unnest-node.cc
M be/src/exec/unnest-node.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/fragment-instance-state.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/krpc-data-stream-sender.h
41 files changed, 226 insertions(+), 141 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/18/15318/2
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 2:
rebased
--
To view, visit http://gerrit.cloudera.org:8080/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 28 Feb 2020 18:27:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15318 )
Change subject: IMPALA-4080 [part 5]: Invoke close on Exprs from plan nodes and sink configs where they were created
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/5393/ : 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/15318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b79d8b8f8e740609676dd13bd300a5514c65b0d
Gerrit-Change-Number: 15318
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Mar 2020 01:13:07 +0000
Gerrit-HasComments: No