impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4014: Introduce query-wide execution state.
Date Tue, 06 Dec 2016 07:58:25 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4014: Introduce query-wide execution state.

Patch Set 7:

File be/src/runtime/

PS7, Line 938: !
nit: missing space
File be/src/runtime/coordinator.h:

PS7, Line 507: fragment_instance_state
instance_state to match below and the definition.

PS7, Line 511: fragment_instance_state
File be/src/runtime/

Line 75: // including the final status when execution finishes.
I had deleted this comment because it wasn't accurate (and had added the accurate info the
the header comment).  Let's not add it back.
File be/src/runtime/fragment-instance-state.h:

Line 53: /// - move ReportStatusCb() logic into PFE::SendReport() and get rid of the callback
thanks, these four are going to make things a lot better.
File be/src/runtime/

Line 115:   // always decrement refcount
not sure what this comment is trying to tell me.  would be more informative as: decrement
the refcount taken in StartFInstance()

Line 148:     qs = it->second;
it's probably worth commenting here explaining why we need to get a new qs pointer, i.e. someone
else may have gc'd the old one and then someone else created a new one.  it's as subtle as
the other cases that are commented.
File be/src/runtime/query-exec-mgr.h:

Line 38: /// entry point for gaining refcounted access to a QueryState.
this doesn't really explain that it's also the thing that executes instances.

PS7, Line 47: decrement
this really happens after execution finishes, so would be good to reword.

PS7, Line 53: CancelFInstance
what's that? (not a method of this class)
File be/src/runtime/query-state.h:

PS7, Line 79: desc_tbl
do we plan to pull this out then? (maybe it's mentioned somewhere i haven't read yet).

Line 83:   bool released_resources() const { return released_resources_; }
who outside this class will care about this? besides, in order to call these methods, you
have to have a refcnt, which means the resources couldn't have been released, right?

Line 93:   /// Illegal to call any other function of this class after this call returns.
Isn't it more that a reference must be held when calling the other methods, and that this
method can't be called until all references are returned? why not specify that?
File be/src/runtime/

Line 90:   DCHECK(status.ok()) << status.GetDetail();
not for this change but we should clean this up.

Line 345: }
are any of these on critical paths (i.e. does the call instruction you're adding matter)?
File be/src/runtime/runtime-state.h:

Line 354:   /// TODO: get rid of this and use ExecEnv::GetInstance() instead
File be/src/service/

Line 68:   fis->Cancel();
is it intentional that we ignore the status returned by Cancel()?  We used to propagate back
as the return_val status (inside CancelPlanFragment).
File be/src/service/query-exec-state.h:

Line 55: /// TODO: Consider renaming to RequestExecState for consistency.
don't do it as part of this change, but this renaming is more important now given the other
similarly named class.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Sailesh Mukil <>
Gerrit-HasComments: Yes

View raw message