This is an automated email from the ASF dual-hosted git repository.
michaelsmith pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new 54642b254 IMPALA-12878: Report invalid query if finalized
54642b254 is described below
commit 54642b2549c38625c868f09815b61170e568cdb9
Author: Michael Smith <[email protected]>
AuthorDate: Wed Mar 6 14:00:18 2024 -0800
IMPALA-12878: Report invalid query if finalized
Fixes a case introduced by IMPALA-12493 where a query is closed again in
the middle of unregistering - which appears to be possible with
cancel_query_and_validate_state - returns "Query not yet running"
instead of "Invalid or unknown query handle".
IMPALA-12493 moved checks for whether a query is inflight before an
atomic CompareAndSwap to mark a QueryDriver as finalized. As part of
that change, a query that was Finalized and removed from
inflight_queries - but not yet removed from query_driver_map_ - could
report "Query not yet running". Updates the check for that error to also
verify the query has not been finalized; if it has, we let the next
conditional handle that case and return "Invalid or unknown query
handle".
Also ensures cancelled queries are closed in test_web_pages tests so
assertions that num_in_flight_queries == 0 at the start of tests are
valid when running the whole suite serially. This doesn't come up in
automated tests because the cases that care about that assertion are
marked execute_serially, while the cases that failed to close a query
were not.
Testing: adds a test to test_cancellation that ensures we get the
expected "Invalid or unknown query handle" when close() is delayed in
calling QueryDriver::Finalize.
Change-Id: I3bf910f499147a09352f9dcb755037b0d8616dfd
Reviewed-on: http://gerrit.cloudera.org:8080/21110
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
---
be/src/runtime/query-driver.cc | 5 ++++-
be/src/service/impala-server.cc | 4 ++++
tests/query_test/test_cancellation.py | 28 ++++++++++++++++++++++++++++
tests/webserver/test_web_pages.py | 1 +
4 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/be/src/runtime/query-driver.cc b/be/src/runtime/query-driver.cc
index 15e3c0e19..edd18c49d 100644
--- a/be/src/runtime/query-driver.cc
+++ b/be/src/runtime/query-driver.cc
@@ -494,7 +494,10 @@ void QueryDriver::HandleRetryFailure(Status* status,
string* error_msg,
Status QueryDriver::Finalize(
QueryHandle* query_handle, bool check_inflight, const Status* cause) {
- if (check_inflight && !(*query_handle)->is_inflight()) {
+ // If the query's not inflight yet, return an appropriate error. If the query
+ // has been finalized and removed from inflight_queries (but not yet removed
+ // from query_driver_map_) we want to fall-through to the next check.
+ if (check_inflight && !(*query_handle)->is_inflight() && !finalized_.Load())
{
return Status("Query not yet running");
}
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index ef3c29f36..3a307e1de 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -1533,6 +1533,10 @@ Status ImpalaServer::UnregisterQuery(const TUniqueId&
query_id, bool check_infli
QueryHandle query_handle;
RETURN_IF_ERROR(GetActiveQueryHandle(query_id, &query_handle));
+ if (check_inflight) {
+ DebugActionNoFail(query_handle->query_options(),
"FINALIZE_INFLIGHT_QUERY");
+ }
+
// Do the work of unregistration that needs to be done synchronously. Once
// Finalize() returns, the query is considered unregistered from the
client's point of
// view. If Finalize() returns OK, this thread is responsible for doing the
diff --git a/tests/query_test/test_cancellation.py
b/tests/query_test/test_cancellation.py
index e4b718b47..21fc59497 100644
--- a/tests/query_test/test_cancellation.py
+++ b/tests/query_test/test_cancellation.py
@@ -25,6 +25,7 @@ import threading
from time import sleep
from RuntimeProfile.ttypes import TRuntimeProfileFormat
from tests.beeswax.impala_beeswax import ImpalaBeeswaxException
+from tests.common.test_dimensions import add_mandatory_exec_option
from tests.common.test_vector import ImpalaTestDimension
from tests.common.impala_test_suite import ImpalaTestSuite
from tests.util.cancel_util import cancel_query_and_validate_state
@@ -275,3 +276,30 @@ class TestCancellationFullSort(TestCancellation):
def test_cancel_sort(self, vector):
self.execute_cancel_test(vector)
+
+
+class TestCancellationFinalizeDelayed(ImpalaTestSuite):
+ @classmethod
+ def get_workload(self):
+ return 'tpch'
+
+ @classmethod
+ def add_test_dimensions(cls):
+ super(TestCancellationFinalizeDelayed, cls).add_test_dimensions()
+
+ # Test with a small delay in QueryDriver::Finalize from close() to check
that queries
+ # that are finalized as a result of closing the session correctly return
"Invalid or
+ # unknown query handle" to the close() RPC.
+ add_mandatory_exec_option(cls, 'debug_action',
'FINALIZE_INFLIGHT_QUERY:SLEEP@10')
+
+ # Debug action is independent of file format, so only testing for
+ # table_format=parquet/none in order to avoid a test dimension explosion.
+ cls.ImpalaTestMatrix.add_constraint(lambda v:
+ v.get_value('table_format').file_format == 'parquet'
+ and v.get_value('table_format').compression_codec == 'none')
+
+ def test_cancellation(self, vector):
+ query = "select l_returnflag from tpch_parquet.lineitem"
+ cancel_delay = 0
+ cancel_query_and_validate_state(self.client, query,
+ vector.get_value('exec_option'), vector.get_value('table_format'),
cancel_delay)
diff --git a/tests/webserver/test_web_pages.py
b/tests/webserver/test_web_pages.py
index 9757b0d72..61e8990c9 100644
--- a/tests/webserver/test_web_pages.py
+++ b/tests/webserver/test_web_pages.py
@@ -537,6 +537,7 @@ class TestWebPage(ImpalaTestSuite):
response_json = json.loads(responses[0].text)
finally:
self.client.cancel(query_handle)
+ self.client.close_query(query_handle)
return response_json
@pytest.mark.xfail(run=False, reason="IMPALA-8059")
