impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/4163/4/be/src/service/child-query.cc
File be/src/service/child-query.cc:

PS4, Line 158: std
> remove std::
Done


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.


http://gerrit.cloudera.org:8080/#/c/4163/4/be/src/service/child-query.h
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
b
Works for me.


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


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


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."


http://gerrit.cloudera.org:8080/#/c/4163/4/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:

PS4, Line 440: setup
> nit: set up
Done


http://gerrit.cloudera.org:8080/#/c/4163/4/be/src/service/query-exec-state.h
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.
Done


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


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe3024803e03595ee69c47759b58e8443d7bd167
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message