Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12530 )
Change subject: IMPALA-8158: Retrieve thrift profiles through Impyla 0.15.0 ...................................................................... Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/12530/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12530/8//COMMIT_MSG@19 PS8, Line 19: This change also makes a change to update the query operation state to See my comment elsewhere. My understanding is that currently the state transition is asynchronous, but should happen without any other client intervention. If you have a counterexample to that, that's really interesting. http://gerrit.cloudera.org:8080/#/c/12530/8/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/12530/8/be/src/service/impala-hs2-server.cc@693 PS8, Line 693: Status status = Status::Expected("Cancelled by client"); This fix isn't quite right, although the current behaviour is confusing and imperfect in some ways - client initiated cancellation goes through a different path in CancelInternal() that sets coordinator_->exec_status_ to Status::CANCELLED, which then gets bubbled up to the query status. I'm not sure off the top of my head why it is propagated differently from other errors, but I think there's some reason. It does seem like we could pass in Status::CANCELLED here and get the "right" behaviour., but I'm probably missing something subtle. Maybe there was some idea that the status shouldn't transition until we've made some effort to clean up the backends, but the same reasoning would apply to cancellation upon errors. So we should probably revisit this. Bikram knows this code fairly well so he might have some insight. -- To view, visit http://gerrit.cloudera.org:8080/12530 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I769a99f0843297dd2b20f2f5b1a9046c97bb131e Gerrit-Change-Number: 12530 Gerrit-PatchSet: 8 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sat, 13 Apr 2019 02:51:13 +0000 Gerrit-HasComments: Yes
