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-4504: fix races in PlanFragmentExecutor regarding status reporting
Date Wed, 30 Nov 2016 19:17:18 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4504: fix races in PlanFragmentExecutor regarding status reporting
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5250/6/be/src/runtime/plan-fragment-executor.cc
File be/src/runtime/plan-fragment-executor.cc:

Line 93:   if (!status.ok() && !report_status_cb_.empty()) {
> There's a bug here. If PrepareInternal() fails after AcquireThreadToken(), 
Yeah, but that's also how the old code worked.  It's benign since in case of failure the resource
pool will go away shortly, but I agree it's best to address this (and it's the original reason
why I made Exec() always call FragmentComplete(), whereas it used to not on failure).


PS6, Line 94: // FragmentComplete() depends on Prepare() having executed successfully, so
must
            :     // call report_status_cb_ directly if Prepare() failed.
> Is this only because of the usage of per_host_mem_usage_ in SendReport()? I
Also the profile() is in an "undefined" state.  Let me fix this up.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I052b7b4fabb341ad27ad294cd5b0a53728d87d0e
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message