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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
