Tim Armstrong has posted comments on this change. Change subject: IMPALA-4037,IMPALA-4038: fix locking during query cancellation ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/4163/5/be/src/service/child-query.cc File be/src/service/child-query.cc: PS5, Line 171: lock_guard<SpinLock> lock(lock_); > you can avoid this if status.ok(), I think. Not sure if it's worth it. I can combine this block with the if() below, works out shorter. Line 185: if (thread == NULL) return Status::OK(); > DCHECK(is_cancelled_)? I think that's the only time that thread == nullptr. It's also false if we never ran any child queries. Added DCHECK(!is_running_). http://gerrit.cloudera.org:8080/#/c/4163/5/be/src/service/child-query.h File be/src/service/child-query.h: PS5, Line 148: is > must be Done PS5, Line 178: Exec > Is there a more descriptive name than Exec()? Every class seems to have an Done Line 194: bool is_running_; > Comment here that, once started, the queries must be allowed to complete or Done http://gerrit.cloudera.org:8080/#/c/4163/5/be/src/service/query-exec-state.h File be/src/service/query-exec-state.h: PS5, Line 226: Acquires > Acquirers Done PS5, Line 227: acquired > required I think I actually intended acquired, but "required" also works. -- 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: 5 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
