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

Reply via email to