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 Thu, 06 Jul 2017 00:27:49 GMT
Tim Armstrong has posted comments on this change.

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

Patch Set 10:

File be/src/exec/

Line 150:   DCHECK(status != nullptr);
> DCHECK(have_async_build_trhead_);

Line 198:       RETURN_IF_ERROR(AcquireResourcesForBuild(state));
> why do we do the build Open() here, rather than keep it in ProcessBuildInpu
Done. The no-sink case will go away with the old aggs and joins fortunately.
File be/src/exec/blocking-join-node.h:

PS10, Line 69: it
> nit: period

PS10, Line 70: it it is started,
> garbled

PS10, Line 109: can be safely closed early
> additionally, aren't we now requiring that it be closed early (i.e. the fro
Yeah the planner depends on this. I updated the comment to mention that and hopefully be a
bit clearer.

Line 121:   /// Processes the build-side input.
> let's also say that the build side should already have been opened (though 

Line 141:   Status SendBuildInputToSink(RuntimeState* state, DataSink* build_sink);
> shouldn't this be private? i.e. we don't expect a derived class to call it 

Line 222:   /// is used for the build. Otherwise, ProcessBuildInput() is called on the subclass.
> explain that this opens the build
File be/src/exec/exec-node.h:

PS10, Line 87: in general
> why? are there cases this isn't true?
I think the only exception is subplans. Updated.
File be/src/exec/

PS10, Line 181:  right away
> what do we mean by "right away". It seemed like we were acquiring them righ
The important this is that it's before the build. Updated the comment accordingly.

To view, visit
To unsubscribe, visit

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

View raw message