Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8997 )
Change subject: IMPALA-6338: Fix flaky test_profile_fragment_instances ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@206 PS2, Line 206: either because the client called Cancel() or because the coordinator : /// initiated cancel because all the results were already returned It could also be because another backend hit an error. Do we know of any other ways cancellation can be initiated? See more in my comment below. http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator-backend-state.h@243 PS2, Line 243: other than cancel We're making a base assumption that a CANCELLED status is only obtained as a result of the Coordinator setting and propagating it (for any reason). But are we sure that's true (It would be ideal if that were true)? i.e. are we sure that there's no fragment instance cancelling itself (and hence the query) for some other reason? Because if that was the case, then with this patch, a fragment instance trying to cancel the query wouldn't cause the query to be cancelled. Since the coordinator is assuming that if it sees a CANCELLED status from a fragment instance, that's due to the coordinator setting it. http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8997/2/be/src/runtime/coordinator.cc@926 PS2, Line 926: // If the query was cancelled by the user, don't process the update. : if (query_status_.IsCancelled()) return Status::OK(); Shouldn't we tell the backend to cancel itself if the query was cancelled by the user? Or are we relying on the Cancel() RPCs doing that? -- To view, visit http://gerrit.cloudera.org:8080/8997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I77773a1e3c62952003f37f88fe2b662bb11889ed Gerrit-Change-Number: 8997 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Sat, 20 Jan 2018 00:45:25 +0000 Gerrit-HasComments: Yes
