Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9501 )
Change subject: IMPALA-6614: ClientRequestState should use HS2 TOperationState ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/9501/3/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/9501/3/be/src/service/client-request-state.cc@a880 PS3, Line 880: > should we check for is_cancelled_ instead to be more explicit and in sync w I think the code is conceptually doing the right thing. If we've reached a terminal state (e.g. EOS or returned an error), then we shouldn't transition to the cancelled state in terms of the CRS state machine. My guess is that the "cancelled" in the comment is referring to the cancellation of the execution, which will happen in all of those cases (i.e EOS, error, user initiated cancellation). I'll update the comment to reflect that. http://gerrit.cloudera.org:8080/#/c/9501/3/be/src/service/client-request-state.cc@a884 PS3, Line 884: > maybe mark a cancelled event even if it was cancelled by the user explicitl I think that makes sense, but as stated in the commit message, I want this change to be completely transparent to the outside world. There will be client/user visible changes, but I'd like to tackle that separately. -- To view, visit http://gerrit.cloudera.org:8080/9501 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I36287eaf8f1dac23c306b470f95f379dfdc6bb5b Gerrit-Change-Number: 9501 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Thu, 08 Mar 2018 18:24:45 +0000 Gerrit-HasComments: Yes
