impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4862: make resource profile consistent with backend behaviour
Date Wed, 21 Jun 2017 21:19:53 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4862: make resource profile consistent with backend behaviour

Patch Set 5:


I found a handful of subtle bugs. Most of them were caught by running tests on my buffer pool
development branch and hittings DCHECKs that too many reservations were claimed. The parallel
plan bugs were found by inspection: resources required by the parallel plans should always
be <= 2x the resources required by the distributed plans.
File be/src/exec/

Line 156:   if (status->ok()) *status = AcquireResourcesForBuild(state);
I added AcquireResourcesForBuild() to defer consuming resources until the build side is open.

Line 169:   if (CanCloseBuildEarly() || !status->ok()) child(1)->Close(state);
I added this to close the build side ASAP when possible.
File be/src/exec/

Line 170:   // Unless we are inside a subplan expecting to call Open()/GetNext() on the child
Moving this wasn't strictly necessary but it seems to make sense to close it once we're done
with it.
File fe/src/main/java/org/apache/impala/planner/

Line 103:   public boolean isBlockingNode() { return false; }
This was also a bug. It's a streaming node! Added a planner test with a pipeline of analytics
that makes it obvious.
File fe/src/main/java/org/apache/impala/planner/

Line 239:     if (sink_ instanceof JoinBuildSink) {
This was another subtle bug - we were double-counting some fragments in the parallel plans
because the join build fragment was included in the parent fragment (there isn't an exchange
node separating the fragments)/.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I492cf5052bb27e4e335395e2a8f8a3b07248ec9d
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message