Tim Armstrong has posted comments on this change. Change subject: IMPALA-4862: make resource profile consistent with backend behaviour ......................................................................
Patch Set 10: (10 comments) 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_); Done 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. 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 Done PS10, Line 70: it it is started, > garbled Done 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 Done 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 Done Line 222: /// is used for the build. Otherwise, ProcessBuildInput() is called on the subclass. > explain that this opens the build Done 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? I think the only exception is subplans. Updated. 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 righ The important this is that it's before the build. Updated the comment accordingly. -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
