Tim Armstrong has posted comments on this change. Change subject: IMPALA-4925: Cancel finstance if query has finished ......................................................................
Patch Set 1: (2 comments) The code change makes sense now that I've wrapped my head around it. Can we add a test that exercises this code path? E.g. there was a query in the JIRA. http://gerrit.cloudera.org:8080/#/c/5987/1//COMMIT_MSG Commit Message: Line 19: A complete fix would also call CancelInternal() when returned_all_results_ Is there a JIRA to track this. http://gerrit.cloudera.org:8080/#/c/5987/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: Line 1514: if (!returned_all_results_ && !status.ok()) { I feel like it would be clearer if !status.ok() came first so it was visually more obvious that this is an error-handling path. Somewhat relatedly, the comment might be clearer if it was phrased as something like "Start cancellation if there was an error - unless all results are returned, in which case we can safely ignore the instance's status". -- To view, visit http://gerrit.cloudera.org:8080/5987 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59f45e64978c9ab9914b5c33e86009960b4a88c4 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
