Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/15821 )
Change subject: IMPALA-9380: async query unregistration ...................................................................... Patch Set 13: (5 comments) With this, I think the cancel rpc from the debug pages's http endpoint will always return success. maybe add an assert in test_display_src_socket_in_query_cause to check a non-error response returned by the http call to the cancel endpoint. http://gerrit.cloudera.org:8080/#/c/15821/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15821/12//COMMIT_MSG@27 PS12, Line 27: All places that look up both the maps will check : the in-flight map first, and return a reference to : the ClientRequestState, i.e. ignoring the entry in : the query log. nit: can you briefly document this cancellation process in the impala-server class description. http://gerrit.cloudera.org:8080/#/c/15821/12/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/15821/12/be/src/service/client-request-state.h@316 PS12, Line 316: StartUnregister Do you mean Finalize? Here and all other references in comments. http://gerrit.cloudera.org:8080/#/c/15821/12/be/src/service/client-request-state.h@530 PS12, Line 530: CAS super nit: Compare and Swap http://gerrit.cloudera.org:8080/#/c/15821/12/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/15821/12/be/src/service/impala-server.h@715 PS12, Line 715: UnregisterQueryAsync nit: FinishUnregisterQueryAsync , just to imply that this only does the remaining part of the unregister process. http://gerrit.cloudera.org:8080/#/c/15821/12/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/15821/12/be/src/service/impala-server.cc@1177 PS12, Line 1177: if (!status.ok()) { : // Query was not registered, just log the issue and proceed. : VLOG_QUERY << "Failed to unregister query " << PrintId(request_state->query_id()) : << ": " << status.GetDetail(); : } will we ever hit this? Based on the changes in UnregisterQuery() only a single thread that is successful in request_state->Finalize() can issue an UnregisterQueryAsync call. -- To view, visit http://gerrit.cloudera.org:8080/15821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I80027b1baeb4ab453938c0f6357b120f4035ba08 Gerrit-Change-Number: 15821 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 04 May 2020 22:39:50 +0000 Gerrit-HasComments: Yes
