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-4014: Introduce query-wide execution state.
Date Wed, 07 Dec 2016 18:23:30 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 7:

(3 comments)

Other than the remaining questions around the released_resources() bool, this change looks
fine to me.

http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 83:   bool released_resources() const { return released_resources_; }
> the idea with explicit resource release (as opposed to implicit, via some d
Okay, yes, that's what was discussed. And yes, the sentence below (that you've removed) was
part of what led to to these comments. But also the fact that ReleaseResources() call is currently
triggered by refcnt going to 0.  i.e. it looks like the resources are also tied to the refcnt
as well as the control structure.  I suppose you plan to move the ReleaseResources() to a
different point (like when all instance executions are complete, which may happen before all
references go away)?

Even so, given that multiple threads will be accessing the QueryState, I don't see how a single
bool is sufficient to safely determine whether the resources are accessible by code outside
of this class.  So, I'm still not sure what the usefulness of this bool is.  Either we'll
have to make accessing/updating this bool and accessing/tearing-down the resources atomic,
or we'll have rules about when during the query lifecycle the resources are valid (but then
this seems to be less explicit).

maybe we can defer adding the bool until the change that actually makes use of it, so that
getting this change committed doesn't get blocked on this discussion?


Line 93:   /// Illegal to call any other function of this class after this call returns.
> see above.
Thanks, this sentence was part of what led to the comment above.


http://gerrit.cloudera.org:8080/#/c/4418/7/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 90:   DCHECK(status.ok()) << status.GetDetail();
> actually, Init() always returns ok. i'm removing this dcheck.
thanks


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I962ae6b7cb7dc0d07fbb8f70317aeb01d88d400b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message