impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4862: make resource profile consistent with backend behaviour
Date Wed, 05 Jul 2017 18:51:43 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 10:

(10 comments)

I need to read through the frontend code again before commenting on that, but here's some
comments on the backend part.

http://gerrit.cloudera.org:8080/#/c/7223/10/be/src/exec/blocking-join-node.cc
File be/src/exec/blocking-join-node.cc:

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 ProcessBuildInputAndOpenProbe()
like the other cases? between async/sync and sink/no-sink and subplan/no-subplan, there are
a lot of cases to understand.


http://gerrit.cloudera.org:8080/#/c/7223/10/be/src/exec/blocking-join-node.h
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 frontend assumes this)?
hmm, or is this just an optimization in the async case?


Line 121:   /// Processes the build-side input.
let's also say that the build side should already have been opened (though this code is going
away soon).


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 directly, right?


Line 222:   /// is used for the build. Otherwise, ProcessBuildInput() is called on the subclass.
explain that this opens the build


http://gerrit.cloudera.org:8080/#/c/7223/10/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

PS10, Line 87: in general
why? are there cases this isn't true?


http://gerrit.cloudera.org:8080/#/c/7223/10/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

PS10, Line 181:  right away
what do we mean by "right away". It seemed like we were acquiring them right away before (but
too soon), no?


-- 
To view, visit http://gerrit.cloudera.org:8080/7223
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I492cf5052bb27e4e335395e2a8f8a3b07248ec9d
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message