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/19 03:03:49 UTC
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10989
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
IMPALA-7256: Aggregator mem usage isn't reflected in summary
This patch fixes a bug where memory used by an Aggregator wasn't being
reflected in the exec summary for the corresponding aggregation node
by ensuring that the Aggregator's MemTracker is a child of the node's
MemTracker.
Testing:
- Manually ran a query and checked that all memory used by the
Aggregator is accounted for in the exec summary.
Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
---
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.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/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
7 files changed, 25 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/10989/1
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10989
to look at the new patch set (#2).
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
IMPALA-7256: Aggregator mem usage isn't reflected in summary
This patch fixes a bug where memory used by an Aggregator wasn't being
reflected in the exec summary for the corresponding aggregation node
by ensuring that the Aggregator's MemTracker is a child of the node's
MemTracker.
Testing:
- Manually ran a query and checked that all memory used by the
Aggregator is accounted for in the exec summary.
Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
---
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/exec-node.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
7 files changed, 23 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/10989/2
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10989 )
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 17:45:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
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/10989 )
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
IMPALA-7256: Aggregator mem usage isn't reflected in summary
This patch fixes a bug where memory used by an Aggregator wasn't being
reflected in the exec summary for the corresponding aggregation node
by ensuring that the Aggregator's MemTracker is a child of the node's
MemTracker.
Testing:
- Manually ran a query and checked that all memory used by the
Aggregator is accounted for in the exec summary.
Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Reviewed-on: http://gerrit.cloudera.org:8080/10989
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/aggregator.cc
M be/src/exec/aggregator.h
M be/src/exec/exec-node.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/grouping-aggregator.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
7 files changed, 23 insertions(+), 10 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10989 )
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 20:59:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10989 )
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10989/1/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/10989/1/be/src/exec/exec-node.cc@113
PS1, Line 113: expr_mem_tracker_.reset(new MemTracker(-1, "Exprs", mem_tracker_.get(), false));
> The parallel MemTracker/ReservationTracker hierarchy is pretty annoying her
Done
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 23:38:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10989 )
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10989/1/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:
http://gerrit.cloudera.org:8080/#/c/10989/1/be/src/exec/exec-node.cc@113
PS1, Line 113: reservation_tracker_.reset(new ReservationTracker);
The parallel MemTracker/ReservationTracker hierarchy is pretty annoying here. The reservation reporting won't work properly if ExecNode::reservation_tracker_ and ExecNode::buffer_pool_client_ are both in use because MemTracker assumes it only has one linked ReservationTracker.
The design of the trackers feels suboptimal to me but we ended up in this place for some valid reasons.
Anyway, I'm thinking it's probably best if we just add reservation_tracker_ to the aggregation nodes where it's actually used so that it's more obvious that this potential wonkiness doesn't happen.
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 21:18:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10989 )
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2844/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 17:45:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7256: Aggregator mem usage isn't reflected in summary
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10989 )
Change subject: IMPALA-7256: Aggregator mem usage isn't reflected in summary
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10989
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iba6ef207bed47810fc742aec3481db5f313cf97f
Gerrit-Change-Number: 10989
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Jul 2018 23:46:55 +0000
Gerrit-HasComments: No