IMPALA-3785: Record query handle for invalid handle Add the query handle to error messages for Invalid Query Handle for beeswax and HS2 interfaces.
Change-Id: Ibc113b3673e1b90f81e80e841740b8006bfd31ba Reviewed-on: http://gerrit.cloudera.org:8080/5748 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/39987d9b Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/39987d9b Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/39987d9b Branch: refs/heads/master Commit: 39987d9bd130cb83483215d31197ec3c9a719b9d Parents: c452595 Author: Zachary Amsden <[email protected]> Authored: Thu Jan 19 21:38:56 2017 +0000 Committer: Impala Public Jenkins <[email protected]> Committed: Sat Feb 4 04:17:06 2017 +0000 ---------------------------------------------------------------------- be/src/service/impala-beeswax-server.cc | 17 +++++++++---- be/src/service/impala-hs2-server.cc | 36 ++++++++++++++++++---------- be/src/util/debug-util-test.cc | 11 +++++++++ tests/comparison/leopard/report.py | 2 +- tests/hs2/test_hs2.py | 17 +++++++++++++ 5 files changed, 64 insertions(+), 19 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/39987d9b/be/src/service/impala-beeswax-server.cc ---------------------------------------------------------------------- diff --git a/be/src/service/impala-beeswax-server.cc b/be/src/service/impala-beeswax-server.cc index ad4963b..bd4ee67 100644 --- a/be/src/service/impala-beeswax-server.cc +++ b/be/src/service/impala-beeswax-server.cc @@ -189,8 +189,9 @@ void ImpalaServer::get_results_metadata(ResultsMetadata& results_metadata, QueryHandleToTUniqueId(handle, &query_id); VLOG_QUERY << "get_results_metadata(): query_id=" << PrintId(query_id); shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, true); - if (exec_state.get() == NULL) { - RaiseBeeswaxException("Invalid query handle", SQLSTATE_GENERAL_ERROR); + if (UNLIKELY(exec_state.get() == nullptr)) { + RaiseBeeswaxException(Substitute("Invalid query handle: $0", PrintId(query_id)), + SQLSTATE_GENERAL_ERROR); } { @@ -249,7 +250,8 @@ beeswax::QueryState::type ImpalaServer::get_state(const QueryHandle& handle) { return entry->second->query_state(); } else { VLOG_QUERY << "ImpalaServer::get_state invalid handle"; - RaiseBeeswaxException("Invalid query handle", SQLSTATE_GENERAL_ERROR); + RaiseBeeswaxException(Substitute("Invalid query handle: $0", PrintId(query_id)), + SQLSTATE_GENERAL_ERROR); } // dummy to keep compiler happy return beeswax::QueryState::FINISHED; @@ -454,7 +456,9 @@ inline void ImpalaServer::QueryHandleToTUniqueId(const QueryHandle& handle, Status ImpalaServer::FetchInternal(const TUniqueId& query_id, const bool start_over, const int32_t fetch_size, beeswax::Results* query_results) { shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, false); - if (exec_state == NULL) return Status("Invalid query handle"); + if (UNLIKELY(exec_state == nullptr)) { + return Status(Substitute("Invalid query handle: $0", PrintId(query_id))); + } // Make sure QueryExecState::Wait() has completed before fetching rows. Wait() ensures // that rows are ready to be fetched (e.g., Wait() opens QueryExecState::output_exprs_, @@ -512,7 +516,10 @@ Status ImpalaServer::FetchInternal(const TUniqueId& query_id, Status ImpalaServer::CloseInsertInternal(const TUniqueId& query_id, TInsertResult* insert_result) { shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, true); - if (exec_state == NULL) return Status("Invalid query handle"); + if (UNLIKELY(exec_state == nullptr)) { + return Status(Substitute("Invalid query handle: $0", PrintId(query_id))); + } + Status query_status; { lock_guard<mutex> l(*exec_state->lock(), adopt_lock_t()); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/39987d9b/be/src/service/impala-hs2-server.cc ---------------------------------------------------------------------- diff --git a/be/src/service/impala-hs2-server.cc b/be/src/service/impala-hs2-server.cc index 5313b7d..51a04f2 100644 --- a/be/src/service/impala-hs2-server.cc +++ b/be/src/service/impala-hs2-server.cc @@ -180,7 +180,9 @@ void ImpalaServer::ExecuteMetadataOp(const THandleIdentifier& session_handle, Status ImpalaServer::FetchInternal(const TUniqueId& query_id, int32_t fetch_size, bool fetch_first, TFetchResultsResp* fetch_results) { shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, false); - if (exec_state == NULL) return Status("Invalid query handle"); + if (UNLIKELY(exec_state == nullptr)) { + return Status(Substitute("Invalid query handle: $0", PrintId(query_id))); + } // FetchResults doesn't have an associated session handle, so we presume that this // request should keep alive the same session that orignated the query. @@ -632,9 +634,10 @@ void ImpalaServer::GetOperationStatus(TGetOperationStatusResp& return_val, VLOG_ROW << "GetOperationStatus(): query_id=" << PrintId(query_id); shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, false); - if (exec_state.get() == nullptr) { + if (UNLIKELY(exec_state.get() == nullptr)) { // No handle was found - HS2_RETURN_ERROR(return_val, "Invalid query handle", SQLSTATE_GENERAL_ERROR); + HS2_RETURN_ERROR(return_val, + Substitute("Invalid query handle: $0", PrintId(query_id)), SQLSTATE_GENERAL_ERROR); } ScopedSessionState session_handle(this); @@ -667,9 +670,10 @@ void ImpalaServer::CancelOperation(TCancelOperationResp& return_val, VLOG_QUERY << "CancelOperation(): query_id=" << PrintId(query_id); shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, false); - if (exec_state.get() == NULL) { + if (UNLIKELY(exec_state.get() == nullptr)) { // No handle was found - HS2_RETURN_ERROR(return_val, "Invalid query handle", SQLSTATE_GENERAL_ERROR); + HS2_RETURN_ERROR(return_val, + Substitute("Invalid query handle: $0", PrintId(query_id)), SQLSTATE_GENERAL_ERROR); } ScopedSessionState session_handle(this); const TUniqueId session_id = exec_state->session_id(); @@ -688,9 +692,10 @@ void ImpalaServer::CloseOperation(TCloseOperationResp& return_val, VLOG_QUERY << "CloseOperation(): query_id=" << PrintId(query_id); shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, false); - if (exec_state.get() == NULL) { + if (UNLIKELY(exec_state.get() == nullptr)) { // No handle was found - HS2_RETURN_ERROR(return_val, "Invalid query handle", SQLSTATE_GENERAL_ERROR); + HS2_RETURN_ERROR(return_val, + Substitute("Invalid query handle: $0", PrintId(query_id)), SQLSTATE_GENERAL_ERROR); } ScopedSessionState session_handle(this); const TUniqueId session_id = exec_state->session_id(); @@ -715,18 +720,22 @@ void ImpalaServer::GetResultSetMetadata(TGetResultSetMetadataResp& return_val, // Look up the session ID (which takes session_state_map_lock_) before taking the query // exec state lock. TUniqueId session_id; - if (!GetSessionIdForQuery(query_id, &session_id)) { - HS2_RETURN_ERROR(return_val, "Invalid query handle", SQLSTATE_GENERAL_ERROR); + if (UNLIKELY(!GetSessionIdForQuery(query_id, &session_id))) { + // No handle was found + HS2_RETURN_ERROR(return_val, + Substitute("Unable to find session ID for query handle: $0", PrintId(query_id)), + SQLSTATE_GENERAL_ERROR); } ScopedSessionState session_handle(this); HS2_RETURN_IF_ERROR(return_val, session_handle.WithSession(session_id), SQLSTATE_GENERAL_ERROR); shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, true); - if (exec_state.get() == NULL) { + if (UNLIKELY(exec_state.get() == nullptr)) { VLOG_QUERY << "GetResultSetMetadata(): invalid query handle"; // No handle was found - HS2_RETURN_ERROR(return_val, "Invalid query handle", SQLSTATE_GENERAL_ERROR); + HS2_RETURN_ERROR(return_val, + Substitute("Invalid query handle: $0", PrintId(query_id)), SQLSTATE_GENERAL_ERROR); } { // make sure we release the lock on exec_state if we see any error @@ -798,9 +807,10 @@ void ImpalaServer::GetLog(TGetLogResp& return_val, const TGetLogReq& request) { request.operationHandle.operationId, &query_id, &secret), SQLSTATE_GENERAL_ERROR); shared_ptr<QueryExecState> exec_state = GetQueryExecState(query_id, false); - if (exec_state.get() == NULL) { + if (UNLIKELY(exec_state.get() == nullptr)) { // No handle was found - HS2_RETURN_ERROR(return_val, "Invalid query handle", SQLSTATE_GENERAL_ERROR); + HS2_RETURN_ERROR(return_val, + Substitute("Invalid query handle: $0", PrintId(query_id)), SQLSTATE_GENERAL_ERROR); } // GetLog doesn't have an associated session handle, so we presume that this request http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/39987d9b/be/src/util/debug-util-test.cc ---------------------------------------------------------------------- diff --git a/be/src/util/debug-util-test.cc b/be/src/util/debug-util-test.cc index 002d628..636d9d4 100644 --- a/be/src/util/debug-util-test.cc +++ b/be/src/util/debug-util-test.cc @@ -20,12 +20,23 @@ #include <iostream> #include "testutil/gtest-util.h" +#include "util/benchmark.h" #include "util/debug-util.h" #include "common/names.h" namespace impala { +TEST(DebugUtil, UniqueID) { + TUniqueId unique_id; + unique_id.hi = 0xfeedbeeff00d7777ULL; + unique_id.lo = 0x2020202020202020ULL; + std::string str("feedbeeff00d7777:2020202020202020"); + EXPECT_EQ(str, PrintId(unique_id)); + unique_id.lo = 0x20ULL; + EXPECT_EQ("feedbeeff00d7777:20", PrintId(unique_id)); +} + string RecursionStack(int level) { if (level == 0) return GetStackTrace(); return RecursionStack(level - 1); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/39987d9b/tests/comparison/leopard/report.py ---------------------------------------------------------------------- diff --git a/tests/comparison/leopard/report.py b/tests/comparison/leopard/report.py index 68487c3..2fd4d77 100644 --- a/tests/comparison/leopard/report.py +++ b/tests/comparison/leopard/report.py @@ -55,7 +55,7 @@ class Report(object): ur'^Column \d+ in row \d+ does not match': 'mismatch', ur'^Could not connect': 'could_not_connect', ur'^IllegalStateException': 'IllegalStateException', - ur'^Invalid query handle': 'invalid_query_handle', + ur'^Invalid query handle: ': 'invalid_query_handle', ur'^Known issue:': 'known_issue', ur'^Operation is in ERROR_STATE': 'error_state', ur'^Query timed out after \d+ seconds': 'timeout', http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/39987d9b/tests/hs2/test_hs2.py ---------------------------------------------------------------------- diff --git a/tests/hs2/test_hs2.py b/tests/hs2/test_hs2.py index 0ed9cd3..c183001 100644 --- a/tests/hs2/test_hs2.py +++ b/tests/hs2/test_hs2.py @@ -227,6 +227,23 @@ class TestHS2(HS2TestSuite): len(operation_handle.operationId.secret)) assert err_msg in get_operation_status_resp.status.errorMessage + @needs_session() + def test_invalid_query_handle(self): + operation_handle = TCLIService.TOperationHandle() + operation_handle.operationId = TCLIService.THandleIdentifier() + operation_handle.operationId.guid = "\x01\x23\x45\x67\x89\xab\xcd\xef76543210" + operation_handle.operationId.secret = "PasswordIsPencil" + operation_handle.operationType = TCLIService.TOperationType.EXECUTE_STATEMENT + operation_handle.hasResultSet = False + + get_operation_status_resp = self.get_operation_status(operation_handle) + TestHS2.check_response(get_operation_status_resp, \ + TCLIService.TStatusCode.ERROR_STATUS) + + print get_operation_status_resp.status.errorMessage + err_msg = "Invalid query handle: efcdab8967452301:3031323334353637" + assert err_msg in get_operation_status_resp.status.errorMessage + @pytest.mark.execute_serially def test_socket_close_forces_session_close(self): """Test that closing the underlying socket forces the associated session to close.
