This is an automated email from the ASF dual-hosted git repository. kturner pushed a commit to branch 1.7 in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/1.7 by this push: new f40f7c1 ACCUMULO-4809 Avoid blocking during session cleanup (#383) f40f7c1 is described below commit f40f7c1ce1d27856d57193fab516c39c85d22d52 Author: Keith Turner <ke...@deenlo.com> AuthorDate: Wed Feb 14 16:17:34 2018 -0500 ACCUMULO-4809 Avoid blocking during session cleanup (#383) --- .../accumulo/tserver/session/SessionManager.java | 78 +++++++++++----------- 1 file changed, 38 insertions(+), 40 deletions(-) diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/session/SessionManager.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/session/SessionManager.java index 78c351d..a03c032 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/session/SessionManager.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/session/SessionManager.java @@ -169,36 +169,38 @@ public class SessionManager { } private void sweep(final long maxIdle, final long maxUpdateIdle) { - List<Session> sessionsToCleanup = new ArrayList<>(); - synchronized (this) { - Iterator<Session> iter = sessions.values().iterator(); - while (iter.hasNext()) { - Session session = iter.next(); - long configuredIdle = maxIdle; - if (session instanceof UpdateSession) { - configuredIdle = maxUpdateIdle; - } - long idleTime = System.currentTimeMillis() - session.lastAccessTime; - if (idleTime > configuredIdle && !session.reserved) { - log.info("Closing idle session from user=" + session.getUser() + ", client=" + session.client + ", idle=" + idleTime + "ms"); - iter.remove(); - sessionsToCleanup.add(session); + // In Accumulo's current code only one thread will ever call this method. However if multiple threads called this method concurrently it could result in + // sessions being lost. This method synchronizes on idleSessions to prevent the loss. Its not expected that anything else will synchronize on idleSessions. + synchronized (idleSessions) { + synchronized (this) { + Iterator<Session> iter = sessions.values().iterator(); + while (iter.hasNext()) { + Session session = iter.next(); + long configuredIdle = maxIdle; + if (session instanceof UpdateSession) { + configuredIdle = maxUpdateIdle; + } + long idleTime = System.currentTimeMillis() - session.lastAccessTime; + if (idleTime > configuredIdle && !session.reserved) { + log.info("Closing idle session from user=" + session.getUser() + ", client=" + session.client + ", idle=" + idleTime + "ms"); + iter.remove(); + idleSessions.add(session); + } } } - } - - // do clean up outside of lock for TabletServer in a synchronized block for simplicity vice a synchronized list - synchronized (idleSessions) { + List<Session> sessionsToCleanup = new ArrayList<>(); - sessionsToCleanup.addAll(idleSessions); - - idleSessions.clear(); + // do clean up outside of lock for TabletServer + for (Session session : idleSessions) { + if (!session.cleanup()) { + sessionsToCleanup.add(session); + } + } - // perform cleanup for all of the sessions - for (Session session : sessionsToCleanup) { - if (!session.cleanup()) - idleSessions.add(session); + synchronized (this) { + idleSessions.clear(); + idleSessions.addAll(sessionsToCleanup); } } } @@ -234,16 +236,14 @@ public class SessionManager { Map<String,MapCounter<ScanRunState>> counts = new HashMap<>(); Set<Entry<Long,Session>> copiedIdleSessions = new HashSet<>(); - synchronized (idleSessions) { - /** - * Add sessions so that get the list returned in the active scans call - */ - for (Session session : idleSessions) { - copiedIdleSessions.add(Maps.immutableEntry(expiredSessionMarker, session)); - } + /** + * Add sessions so that get the list returned in the active scans call + */ + for (Session session : idleSessions) { + copiedIdleSessions.add(Maps.immutableEntry(expiredSessionMarker, session)); } - for (Entry<Long,Session> entry : sessions.entrySet()) { + for (Entry<Long,Session> entry : Iterables.concat(sessions.entrySet(), copiedIdleSessions)) { Session session = entry.getValue(); @SuppressWarnings("rawtypes") @@ -286,13 +286,11 @@ public class SessionManager { final long ct = System.currentTimeMillis(); final Set<Entry<Long,Session>> copiedIdleSessions = new HashSet<>(); - synchronized (idleSessions) { - /** - * Add sessions so that get the list returned in the active scans call - */ - for (Session session : idleSessions) { - copiedIdleSessions.add(Maps.immutableEntry(expiredSessionMarker, session)); - } + /** + * Add sessions so that get the list returned in the active scans call + */ + for (Session session : idleSessions) { + copiedIdleSessions.add(Maps.immutableEntry(expiredSessionMarker, session)); } for (Entry<Long,Session> entry : Iterables.concat(sessions.entrySet(), copiedIdleSessions)) { -- To stop receiving notification emails like this one, please contact ktur...@apache.org.