Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/12926 )
Change subject: IMPALA-7031: Add additional info to query canceled from http endpoint ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-beeswax-server.cc File be/src/service/impala-beeswax-server.cc: http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-beeswax-server.cc@201 PS9, Line 201: ImpalaServer:: nit: you can just call the method without this scope resolution. http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-server.h@605 PS9, Line 605: // When a query is unregistered, its QueryStateRecord will be added into query log : // during the archive stage. This method is used to identify whether the query is : // unregistered and return a proper error message. The method is used for unknown : // query, which is a query lost its ClientRequestState. Returns the appropriate error message for an invalid query id. Also checks whether the query was recently unregistered by checking for it in the query log and adds the appropriate info to the error message. http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-server.h@609 PS9, Line 609: getErrMsgForUnknownQuery nit: how about getErrMsgForInvalidQueryId http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-server.cc@2589 PS9, Line 2589: Query is already unregistered I think we should still say invalid query handle in this case too, because it actually is invalid, the query being unregistered is only extra info http://gerrit.cloudera.org:8080/#/c/12926/9/tests/hs2/test_hs2.py File tests/hs2/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/12926/9/tests/hs2/test_hs2.py@335 PS9, Line 335: : assert "Query is already unregistered" or "Invalid query handle" \ : in get_operation_status_resp.status.errorMessage ("a" or "b" in str) is the wrong conditional, please see the following link as to how it will be interpreted: https://stackoverflow.com/questions/21344842/if-a-or-b-in-l-where-l-is-a-list-python http://gerrit.cloudera.org:8080/#/c/12926/9/tests/query_test/test_cancellation.py File tests/query_test/test_cancellation.py: http://gerrit.cloudera.org:8080/#/c/12926/9/tests/query_test/test_cancellation.py@194 PS9, Line 194: ('Invalid query handle' in str(thread.fetch_results_error) or : 'Query is already unregistered' in str(thread.fetch_results_error) : and not join_before_close) this becomes A || B & C, which is interpreted as A || (B & C) whereas what we want here is (A || B) & C. Can you add the appropriate brackets to fix this http://gerrit.cloudera.org:8080/#/c/12926/9/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/12926/9/tests/webserver/test_web_pages.py@546 PS9, Line 546: add @pytest.mark.execute_serially decorator to this test. You should make sure that "Query is already unregistered" is always returned to know that your patch is working, but in order to flush out any flakiness due to other queries running and pushing out the query state from the query log we can run it serially http://gerrit.cloudera.org:8080/#/c/12926/9/tests/webserver/test_web_pages.py@548 PS9, Line 548: """When a query is successfully canceled, client should be notified that the : query is already unregistered.""" nit:how about: Make sure an indicative error message is returned when trying to use a query handle for a recently cancelled query. That kind of message is returned if the query is still in the query archive log, so to make sure it is not pushed out by other queries we run this test serially. http://gerrit.cloudera.org:8080/#/c/12926/9/tests/webserver/test_web_pages.py@562 PS9, Line 562: # Because the query is unregistered, client should receive exception with : # "Query is already unregistered" or "Invalid query handle" notice. : # For the query gets "Invalid query handle" is because query log might be : # full and then removed the oldest unregistered query nit: you can remove this comment after you make the change to run this serially -- To view, visit http://gerrit.cloudera.org:8080/12926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56983d40e0542bc734ec5a66c339b5131b7b56c8 Gerrit-Change-Number: 12926 Gerrit-PatchSet: 9 Gerrit-Owner: Alice Fan <fan...@gmail.com> Gerrit-Reviewer: Alice Fan <fan...@gmail.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 07 May 2019 20:56:01 +0000 Gerrit-HasComments: Yes
