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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
