impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4037,IMPALA-4038: fix locking during query cancellation
Date Thu, 01 Sep 2016 23:33:32 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4037,IMPALA-4038: fix locking during query cancellation

Patch Set 4:

File be/src/service/

PS4, Line 158: std
> remove std::

Line 181:   is_running_ = false;
> Move into lock scope? And maybe move to Exec(), since it's a postcondition 
Moved into Exec(). I think that works.

Line 195:   Thread* thread;
> = GetChildQueriesThread() ? Or are you worried about taking and yielding th
Reworked this function.

PS4, Line 196: vector<ChildQuery*> child_queries_copy;
> after is_running_ is set, child_queries_ should never change (that should b
I agree it should be safe, I was being conservative.

I added documentation to the header about when fields are immutable and then avoided acquiring
'lock_' in the cases when its unnecessary. I think it worked out simpler on the net.

Line 211:   for (ChildQuery* child_query: child_queries_copy) child_query->Cancel();
> space before :
Rewrote this bit anyway.
File be/src/service/child-query.h:

PS4, Line 153: ChildQueryExecutor();
> If you pass the child queries in at construction you can ensure the invaria
I considered that, but it makes the locking in QES way hairier if ChildQueryExecutor is created
midway through Exec(), since we then need to acquire an external lock to avoid a race in checking
whether child_query_executor_ is non-NULL.

Line 159:   void ExecAsync(std::vector<ChildQuery>* child_queries);
> why not std::vector<ChildQuery>&& to make it clear that the argument must
Works for me.

PS4, Line 175: is blocking
> blocks until all queries are completed

PS4, Line 187: mutex
> SpinLock might be better.

PS4, Line 193: has not been Join()ed
> Join() doesn't imply the thread has stopped (i.e. this seems necessary but 
Are you sure? The comment on Thread::Join() says "Blocks until this thread finishes execution."
File be/src/service/

PS4, Line 440: setup
> nit: set up
File be/src/service/query-exec-state.h:

PS4, Line 130: yet
> Can this be called after the query has finished? If so, remove 'yet'.
Good point. Done.

PS4, Line 227: various ImpalaServer operations
> bit vague - do you mean that service-wide operations may be blocked on gett
Reworded and mentioned IMPALA-3882 (since that's what causes it to really get out of control).

Line 230:   ExecEnv* exec_env_;
> blank line before this one.

Line 338:   Status ExecQueryOrDmlRequest(const TQueryExecRequest& query_exec_request);
> Add a comment that coord() is only valid after this method (if this path is

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe3024803e03595ee69c47759b58e8443d7bd167
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Henry Robinson <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message