Alice Fan 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. Done 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 Done http://gerrit.cloudera.org:8080/#/c/12926/9/be/src/service/impala-server.h@609 PS9, Line 609: getErrMsgForUnknownQuery > nit: how about getErrMsgForInvalidQueryId sure. 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 Done 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 Thanks! Removed changes here. As error msg now will include 'Invalid query handle' in anyway. 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) Woops. Thanks for point this out! I removed the "Query is already unregistered" from here, as it is an extra info for "Invalid query handle" 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 s Thanks! 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 tryin Done 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 seri Done -- 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 <[email protected]> Gerrit-Reviewer: Alice Fan <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 09 May 2019 03:16:12 +0000 Gerrit-HasComments: Yes
