[Impala-ASF-CR] IMPALA-4037,IMPALA-4038: fix locking during query cancellation

["Henry Robinson (Code Review)" <ge...@cloudera.org>]

Henry Robinson has posted comments on this change.

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


Patch Set 4:

(15 comments)

Refactoring looks sensible. Does the stress test run compute stats to exercise the CQ path?

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::


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


Line 195:   Thread* thread;
= GetChildQueriesThread() ? Or are you worried about taking and yielding the lock several times?


PS4, Line 196: vector<ChildQuery*> child_queries_copy;
after is_running_ is set, child_queries_ should never change (that should be an invariant of only calling ExecAsync once). Is it overkill to have this protective locking and copying since several clients could safely read the vector at once?

i.e. if you guarantee to read only after checking is_running_ == true on line 200, the vector is essentially const.


Line 211:   for (ChildQuery* child_query: child_queries_copy) child_query->Cancel();
space before :


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 invariant of only ever executing one set of queries. I don't feel strongly in either direction though.


Line 159:   void ExecAsync(std::vector<ChildQuery>* child_queries);
why not std::vector<ChildQuery>&& to make it clear that the argument must be movable from?


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 not sufficient).


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


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


PS4, Line 227: various ImpalaServer operations
bit vague - do you mean that service-wide operations may be blocked on getting this lock, and that therefore other queries / clients may be affected? If so, good to be explicit about that.


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 used), and it may not be valid if it returns an error.


-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-HasComments: Yes