impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5567: race in fragment instance teardown
Date Mon, 26 Jun 2017 19:58:47 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5567: race in fragment instance teardown

Patch Set 2:

File be/src/exec/

Line 101:     RETURN_IF_ERROR(state->GetQueryStatus());
At first glance this looks reasonable to put here, but is this necessary for this change?
Looks like the caller is FIS::ExecInternal and the error status gets propagated. I'm just
asking because we'll start seeing errors sooner, which seems like a good thing but may result
in some different runtime behavior.
File be/src/runtime/runtime-state.h:

PS1, Line 294:   /// Returns a non-OK status if query execution should stop (e.g., the query
             :   /// cancelled or a mem limit was exceeded). Exec nodes should check this
periodically so
             :   /// execution doesn't continue if the query terminates abnormally. This should
not be
             :   /// called after ReleaseResources().
             :   Status CheckQueryState();
> I wonder if most of the remaining callers can be converted to CheckQuerySta
I like CheckAsyncError() for GetQueryStatus(). If we can convert all of the other callsites
to CheckQueryStatus(), that'd clearly be ideal, but that seems like a bigger change to move
Allocate -> TryAllocate. However, maybe we can split CheckQueryState() into a fn that just
updates the status_ on mem limit exceeded, then the places where we know the memory may have
been over consumed we call that before calling CheckAsyncError()? I guess CheckMemLimitOrAsyncError()
is at least more clear for now. I'm OK with fixing this more localized issue now.

It's funny, CheckQueryStatus() clearly suffers from the QueryMaintenance() problem.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If567e734042b5f2b82323368dd536dbf3bdf4744
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message