Repository: incubator-impala Updated Branches: refs/heads/master b7eeb8bf8 -> 4db330e69
IMPALA-4516: Don't hold process wide lock connection_to_sessions_map_lock_ while cancelling queries We hold the connection_to_sessions_map_lock_ while closing multiple sessions, which could map to a large number of queries, which means an even larger number of fragments. We hold this process wide lock and a series of other locks while sending cancel RPCs to all the fragments that fall under the above mentioned category. This could slow down the responsiveness to the client by the daemon. Moreover, holding the lock is unnecessary and we can do without it. This patch fixes this path by dropping the lock before we call CloseSessionInternal(). Change-Id: I9fe37955027ad9fec3fdbbbcb199245c79bcac71 Reviewed-on: http://gerrit.cloudera.org:8080/5173 Reviewed-by: Sailesh Mukil <[email protected]> Tested-by: Internal 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/4821ea6c Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/4821ea6c Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/4821ea6c Branch: refs/heads/master Commit: 4821ea6c468d3d4ae37d47b21aafaee553fb0549 Parents: b7eeb8b Author: Sailesh Mukil <[email protected]> Authored: Mon Nov 21 12:51:43 2016 -0800 Committer: Internal Jenkins <[email protected]> Committed: Tue Nov 22 22:26:58 2016 +0000 ---------------------------------------------------------------------- be/src/service/impala-server.cc | 24 ++++++++++++++++-------- be/src/service/impala-server.h | 18 +++++++++--------- 2 files changed, 25 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4821ea6c/be/src/service/impala-server.cc ---------------------------------------------------------------------- diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc index 49e81c8..5e08224 100644 --- a/be/src/service/impala-server.cc +++ b/be/src/service/impala-server.cc @@ -1654,24 +1654,32 @@ void ImpalaServer::ConnectionStart( void ImpalaServer::ConnectionEnd( const ThriftServer::ConnectionContext& connection_context) { - unique_lock<mutex> l(connection_to_sessions_map_lock_); - ConnectionToSessionMap::iterator it = - connection_to_sessions_map_.find(connection_context.connection_id); - // Not every connection must have an associated session - if (it == connection_to_sessions_map_.end()) return; + vector<TUniqueId> sessions_to_close; + { + unique_lock<mutex> l(connection_to_sessions_map_lock_); + ConnectionToSessionMap::iterator it = + connection_to_sessions_map_.find(connection_context.connection_id); + + // Not every connection must have an associated session + if (it == connection_to_sessions_map_.end()) return; + + // We don't expect a large number of sessions per connection, so we copy it, so that + // we can drop the map lock early. + sessions_to_close = it->second; + connection_to_sessions_map_.erase(it); + } LOG(INFO) << "Connection from client " << connection_context.network_address - << " closed, closing " << it->second.size() << " associated session(s)"; + << " closed, closing " << sessions_to_close.size() << " associated session(s)"; - for (const TUniqueId& session_id: it->second) { + for (const TUniqueId& session_id: sessions_to_close) { Status status = CloseSessionInternal(session_id, true); if (!status.ok()) { LOG(WARNING) << "Error closing session " << session_id << ": " << status.GetDetail(); } } - connection_to_sessions_map_.erase(it); } void ImpalaServer::RegisterSessionTimeout(int32_t session_timeout) { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4821ea6c/be/src/service/impala-server.h ---------------------------------------------------------------------- diff --git a/be/src/service/impala-server.h b/be/src/service/impala-server.h index 9d14ecd..d87af39 100644 --- a/be/src/service/impala-server.h +++ b/be/src/service/impala-server.h @@ -78,15 +78,14 @@ class TGetExecSummaryReq; /// This class is partially thread-safe. To ensure freedom from deadlock, if multiple /// locks are acquired, lower-numbered locks must be acquired before higher-numbered /// locks: -/// 1. connection_to_sessions_map_lock_ -/// 2. session_state_map_lock_ -/// 3. SessionState::lock -/// 4. query_expiration_lock_ -/// 5. query_exec_state_map_lock_ -/// 6. QueryExecState::fetch_rows_lock -/// 7. QueryExecState::lock -/// 8. QueryExecState::expiration_data_lock_ -/// 9. Coordinator::exec_summary_lock +/// 1. session_state_map_lock_ +/// 2. SessionState::lock +/// 3. query_expiration_lock_ +/// 4. query_exec_state_map_lock_ +/// 5. QueryExecState::fetch_rows_lock +/// 6. QueryExecState::lock +/// 7. QueryExecState::expiration_data_lock_ +/// 8. Coordinator::exec_summary_lock /// /// Coordinator::lock_ should not be acquired at the same time as the /// ImpalaServer/SessionState/QueryExecState locks. Aside from @@ -99,6 +98,7 @@ class TGetExecSummaryReq; /// * query_locations_lock_ /// * uuid_lock_ /// * catalog_version_lock_ +/// * connection_to_sessions_map_lock_ /// /// TODO: The state of a running query is currently not cleaned up if the /// query doesn't experience any errors at runtime and close() doesn't get called.
