Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15840 )
Change subject: IMPALA-6984: coordinator cancels backends on EOS ...................................................................... Patch Set 3: (5 comments) still trying to understand the code here, but some initial comments http://gerrit.cloudera.org:8080/#/c/15840/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15840/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-6984 there seems to be some concern in https://issues.apache.org/jira/browse/IMPALA-6984 about whether or not the exec summary will be properly updated after making this change, not sure if those concerns are still valid. Thomas might know more since he filed https://issues.apache.org/jira/browse/IMPALA-5783 http://gerrit.cloudera.org:8080/#/c/15840/3//COMMIT_MSG@28 PS3, Line 28: Manually inspected logs of some queries with limit, : saw that it sent cancellation then waited for backends : as expected. would be nice to have a test for this, but could be deferred to a follow JIRA if necessary http://gerrit.cloudera.org:8080/#/c/15840/3/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/15840/3/be/src/runtime/coordinator-backend-state.h@121 PS3, Line 121: Returns true if : /// cancellation was attempted, false otherwise. nit: needs updating http://gerrit.cloudera.org:8080/#/c/15840/3/be/src/runtime/coordinator-backend-state.h@393 PS3, Line 393: bool sent_cancel_rpc_ = false; is there any chance we intentionally decided to *not* add something like this? I think after adding this the impala coordinator can only ever send the cancellation RPC to a backend once, but maybe that is by design? perhaps we intentionally allow users to repeatedly issue cancellation requests so that the cancellation RPCs can be issued multiple times? perhaps because there is some chance the RPCs get dropped somehow a bit of a edge case, but thought i would mention it http://gerrit.cloudera.org:8080/#/c/15840/3/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/15840/3/be/src/runtime/coordinator-backend-state.cc@561 PS3, Line 561: result.became_done = true; this should only be set if the status_ gets set to CANCELLED above, right? so should it be if (status_.ok()) { status_ = Status::CANCELLED; result.became_done = true; } -- To view, visit http://gerrit.cloudera.org:8080/15840 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I966eceaafdc18a019708b780aee4ee9d70fd3a47 Gerrit-Change-Number: 15840 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 04 May 2020 22:54:26 +0000 Gerrit-HasComments: Yes
