Mailing-List: contact commits-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: mjacobs@apache.org
To: commits@impala.incubator.apache.org
Date: Tue, 29 Aug 2017 16:00:52 -0000
Message-Id: <5ebccd455c2e46a9bb42ccb23cefc2d9@git.apache.org>
In-Reply-To: <0b264318b9fc4cd5a4bb4fd3d817d719@git.apache.org>
References: <0b264318b9fc4cd5a4bb4fd3d817d719@git.apache.org>
Subject: [4/7] incubator-impala git commit: IMPALA-5853: fix
 GetResultSetMetadata() error message for invalid query id
archived-at: Tue, 29 Aug 2017 16:00:54 -0000

IMPALA-5853: fix GetResultSetMetadata() error message for invalid query id

When given an invalid query id, most RPCs result with "Invalid query
id", but GetResultSetMetadata() response with "Unable to find session ID
for query handle".  That's confusing; make it consistent with other
RPCs.

Change-Id: I51eecf353f9cecaf88cecc9392b08ac1b9326b66
Reviewed-on: http://gerrit.cloudera.org:8080/7863
Reviewed-by: Dan Hecht <dhecht@cloudera.com>
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/593ec257
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/593ec257
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/593ec257

Branch: refs/heads/master
Commit: 593ec25736bffdea11ffceb645a27f27d2eb73f5
Parents: 77e9e26
Author: Dan Hecht <dhecht@cloudera.com>
Authored: Mon Aug 28 15:25:03 2017 -0700
Committer: Impala Public Jenkins <impala-public-jenkins@gerrit.cloudera.org>
Committed: Tue Aug 29 02:45:00 2017 +0000

----------------------------------------------------------------------
 be/src/service/impala-hs2-server.cc | 17 ++++-------------
 be/src/service/impala-server.cc     | 13 -------------
 be/src/service/impala-server.h      |  4 ----
 tests/hs2/test_hs2.py               | 11 ++++++++++-
 4 files changed, 14 insertions(+), 31 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/593ec257/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 6a1b5f4..f32d04e 100644
--- a/be/src/service/impala-hs2-server.cc
+++ b/be/src/service/impala-hs2-server.cc
@@ -720,19 +720,6 @@ void ImpalaServer::GetResultSetMetadata(TGetResultSetMetadataResp& return_val,
       request.operationHandle.operationId, &query_id, &secret), SQLSTATE_GENERAL_ERROR);
   VLOG_QUERY << "GetResultSetMetadata(): query_id=" << PrintId(query_id);
 
-  // Look up the session ID (which takes session_state_map_lock_) before taking the query
-  // exec state lock.
-  TUniqueId session_id;
-  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<ClientRequestState> request_state = GetClientRequestState(query_id);
   if (UNLIKELY(request_state.get() == nullptr)) {
     VLOG_QUERY << "GetResultSetMetadata(): invalid query handle";
@@ -740,6 +727,10 @@ void ImpalaServer::GetResultSetMetadata(TGetResultSetMetadataResp& return_val,
     HS2_RETURN_ERROR(return_val,
       Substitute("Invalid query handle: $0", PrintId(query_id)), SQLSTATE_GENERAL_ERROR);
   }
+  ScopedSessionState session_handle(this);
+  const TUniqueId session_id = request_state->session_id();
+  HS2_RETURN_IF_ERROR(return_val, session_handle.WithSession(session_id),
+      SQLSTATE_GENERAL_ERROR);
   {
     lock_guard<mutex> l(*request_state->lock());
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/593ec257/be/src/service/impala-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index 41f84b1..fb1d0e6 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -2053,19 +2053,6 @@ Status CreateImpalaServer(ExecEnv* exec_env, int beeswax_port, int hs2_port, int
   return Status::OK();
 }
 
-bool ImpalaServer::GetSessionIdForQuery(const TUniqueId& query_id,
-    TUniqueId* session_id) {
-  DCHECK(session_id != nullptr);
-  lock_guard<mutex> l(client_request_state_map_lock_);
-  ClientRequestStateMap::iterator i = client_request_state_map_.find(query_id);
-  if (i == client_request_state_map_.end()) {
-    return false;
-  } else {
-    *session_id = i->second->session_id();
-    return true;
-  }
-}
-
 shared_ptr<ClientRequestState> ImpalaServer::GetClientRequestState(
     const TUniqueId& query_id) {
   lock_guard<mutex> l(client_request_state_map_lock_);

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/593ec257/be/src/service/impala-server.h
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.h b/be/src/service/impala-server.h
index 344368b..1e83de5 100644
--- a/be/src/service/impala-server.h
+++ b/be/src/service/impala-server.h
@@ -400,10 +400,6 @@ class ImpalaServer : public ImpalaServiceIf, public ImpalaHiveServer2ServiceIf,
   std::shared_ptr<ClientRequestState> GetClientRequestState(
       const TUniqueId& query_id);
 
-  /// Writes the session id, if found, for the given query to the output
-  /// parameter. Returns false if no query with the given ID is found.
-  bool GetSessionIdForQuery(const TUniqueId& query_id, TUniqueId* session_id);
-
   /// Updates the number of databases / tables metrics from the FE catalog
   Status UpdateCatalogMetrics() WARN_UNUSED_RESULT;
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/593ec257/tests/hs2/test_hs2.py
----------------------------------------------------------------------
diff --git a/tests/hs2/test_hs2.py b/tests/hs2/test_hs2.py
index 42c3fb7..f42b29a 100644
--- a/tests/hs2/test_hs2.py
+++ b/tests/hs2/test_hs2.py
@@ -262,10 +262,19 @@ class TestHS2(HS2TestSuite):
     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
 
+    get_result_set_metadata_req = TCLIService.TGetResultSetMetadataReq()
+    get_result_set_metadata_req.operationHandle = operation_handle
+    get_result_set_metadata_resp = \
+        self.hs2_client.GetResultSetMetadata(get_result_set_metadata_req)
+    TestHS2.check_response(get_result_set_metadata_resp, \
+        TCLIService.TStatusCode.ERROR_STATUS)
+
+    err_msg = "Invalid query handle: efcdab8967452301:3031323334353637"
+    assert err_msg in get_result_set_metadata_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.