Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17999 )
Change subject: IMPALA-11006: Impalad crashes during query cancel tests ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/17999/1/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/17999/1/be/src/service/client-request-state.cc@1070 PS1, Line 1070: else What is query_status_ in case of cancellation? Shouldn't it be in an error status, which would lead to returning at line 1058? I looked around and I think that we are quite inconsistent here - Cancel() only updates the query status if there is a "cause": https://github.com/apache/impala/blob/9d2ef8564786d858db7786ad338b7daa5386eb20/be/src/service/client-request-state.cc#L1343 Otherwise only is_cancelled_ will become true. This seems intentional: https://github.com/apache/impala/blob/9d2ef8564786d858db7786ad338b7daa5386eb20/be/src/service/client-request-state.h#L175 But if the cancellation happens at some certain times, we do set query_status_ regardless of having a cause or not: https://github.com/apache/impala/blob/9d2ef8564786d858db7786ad338b7daa5386eb20/be/src/service/client-request-state.cc#L612 I think that this logic would be clearer if we would decide first whether GetCoordinator() should return non-null - my understanding is that there are some ddl-s that still won't have a coordinator. In case the it should have a coordinator but it doesn't, then we could check if it was cancelled and hit a DCHECK if it was not. http://gerrit.cloudera.org:8080/#/c/17999/1/be/src/service/client-request-state.cc@1076 PS1, Line 1076: MarkInactive I think that MarkInactive() is not needed in case of cancellation, as we have already closed the query and expect no more RPCs from the client. Calling MarkInactive() means that Impala is no longer active, but we are waiting for more activity from the client. -- To view, visit http://gerrit.cloudera.org:8080/17999 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia49411f8525734b8d463d9ffbfbce705b90a8d73 Gerrit-Change-Number: 17999 Gerrit-PatchSet: 1 Gerrit-Owner: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Comment-Date: Mon, 08 Nov 2021 13:28:38 +0000 Gerrit-HasComments: Yes
