Henry Robinson has posted comments on this change. Change subject: IMPALA-4037,IMPALA-4038: fix locking during query cancellation ......................................................................
Patch Set 5: Code-Review+1 (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. Line 185: if (thread == NULL) return Status::OK(); DCHECK(is_cancelled_)? I think that's the only time that thread == nullptr. 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 PS5, Line 178: Exec Is there a more descriptive name than Exec()? Every class seems to have an Exec() method... maybe ExecSequentially()? ExecChildQueries()? Line 194: bool is_running_; Comment here that, once started, the queries must be allowed to complete or fail (when the class is destroyed, we dcheck(!is_running); and say what methods force that as a post-condition (WaitForAll(), Cancel()). You have that in the class comment, but worth a mention here as well I think. 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 PS5, Line 227: acquired required -- 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
