You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Marshall (Code Review)" <ge...@cloudera.org> on 2018/07/04 00:32:38 UTC
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10863
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
A recent change, IMPALA-110 (part 2), refactored the aggregation code.
During this refactor, a call to QueryMaintenance() was inadvertantely
dropped in the AddBatchStreaming() path, causing
test_spilling_regression_exhaustive to fail by hitting the memory
limit.
Testing:
- Ran test_spilling_regression_exhaustive
Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
---
M be/src/exec/grouping-aggregator.cc
1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/10863/1
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Jul 2018 20:31:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
Patch Set 1: Code-Review+2
I'm ok with this fix to unblock the test, but we should investigate the other issues as a follow-up blocker to make sure we didn't break anything else.
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Jul 2018 17:12:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc
File be/src/exec/grouping-aggregator.cc:
http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc@423
PS1, Line 423: RETURN_IF_ERROR(QueryMaintenance(state));
expr_results_pool_ is used in AddBatchStreamingImpl(), right ?
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Jul 2018 02:01:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
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/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
A recent change, IMPALA-110 (part 2), refactored the aggregation code.
During this refactor, a call to QueryMaintenance() was inadvertantely
dropped in the AddBatchStreaming() path, causing
test_spilling_regression_exhaustive to fail by hitting the memory
limit.
Testing:
- Ran test_spilling_regression_exhaustive
Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Reviewed-on: http://gerrit.cloudera.org:8080/10863
Reviewed-by: Michael Ho <kw...@cloudera.com>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/grouping-aggregator.cc
1 file changed, 1 insertion(+), 0 deletions(-)
Approvals:
Michael Ho: Looks good to me, but someone else must approve
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2775/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Jul 2018 17:17:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Jul 2018 02:01:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc
File be/src/exec/grouping-aggregator.cc:
http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc@407
PS1, Line 407: SCOPED_TIMER(build_timer_);
Do we have a similar bug here? I'm not sure how expr_results_pool_ gets cleared in the aggregator when driven from Open(). Same for the non-grouping aggregator.
It would be nice to avoid the need for these two sets of QueryMaintenance() calls to clear expr_results_pool_ at both levels. We could consider using ExecNode::expr_results_pool_, which reduces the granularity of some of the memory tracking but I think should be ok - the "result" allocations are expected to be small unless an expr misbehaves.
Otherwise we could have the parent ExecNode() clear the results pools on the aggregators at the same points as it's clearing its own pools to avoid this duplication.
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 04 Jul 2018 02:37:36 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10863 )
Change subject: IMPALA-7240: Fix missing QueryMaintenance call in AddBatchStreaming
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc
File be/src/exec/grouping-aggregator.cc:
http://gerrit.cloudera.org:8080/#/c/10863/1/be/src/exec/grouping-aggregator.cc@407
PS1, Line 407: SCOPED_TIMER(build_timer_);
> Do we have a similar bug here? I'm not sure how expr_results_pool_ gets cle
Yep. Submitted this so to get the builds green, and filed a new review: https://gerrit.cloudera.org/#/c/10871/
Since the ExecNode::expr_results_pool_ isn't being used anyways, my suggestion is to keep Aggregator::expr_results_pool_ and then eliminate the calls to QueryMaintenance() in the exec nodes to get rid of the duplication.
I don't feel strongly about it, though, happy to implement one of your proposed solutions instead.
--
To view, visit http://gerrit.cloudera.org:8080/10863
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I150a46d00acad139d186a150d9706ef47a10ac36
Gerrit-Change-Number: 10863
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Jul 2018 21:22:44 +0000
Gerrit-HasComments: Yes