From 3e510ba17084040f3ba1f68c1234a4d37a69255b Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Mon, 10 Jul 2023 13:41:49 -0400 Subject: [PATCH 1/2] adds unit test for deferred session cleanup --- .../tserver/session/SessionManager.java | 6 +- .../tserver/session/SessionManagerTest.java | 109 ++++++++++++++++++ 2 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java 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 f0f8a5de2a1..5041b5c66f6 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 @@ -224,7 +224,7 @@ public Session removeSession(long sessionId, boolean unreserve) { return session; } - private void cleanup(Session session) { + static void cleanup(BlockingQueue deferredCleanupQueue, Session session) { if (!session.cleanup()) { var retry = Retry.builder().infiniteRetries().retryAfter(25, MILLISECONDS) .incrementBy(25, MILLISECONDS).maxWait(5, SECONDS).backOffFactor(1.5) @@ -248,6 +248,10 @@ private void cleanup(Session session) { } } + private void cleanup(Session session) { + cleanup(deferredCleanupQueue, session); + } + private void sweep(final long maxIdle, final long maxUpdateIdle) { List sessionsToCleanup = new LinkedList<>(); Iterator iter = sessions.values().iterator(); diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java b/server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java new file mode 100644 index 00000000000..be0658dbca1 --- /dev/null +++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java @@ -0,0 +1,109 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.tserver.session; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.concurrent.ArrayBlockingQueue; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class SessionManagerTest { + + private static class TestSession extends Session { + + int cleanupCount; + + TestSession(int cleanupCount) { + super(null); + this.cleanupCount = cleanupCount; + } + + @Override + public boolean cleanup() { + return cleanupCount-- <= 0; + } + } + + @Test + public void testTestcode() { + // test behavior of test class + TestSession session = new TestSession(2); + Assertions.assertFalse(session.cleanup()); + Assertions.assertFalse(session.cleanup()); + Assertions.assertTrue(session.cleanup()); + Assertions.assertTrue(session.cleanup()); + } + + @Test + public void testFullDeferredCleanupQueue() { + ArrayBlockingQueue deferredCleanupQeue = new ArrayBlockingQueue<>(3); + + deferredCleanupQeue.add(new TestSession(2)); + deferredCleanupQeue.add(new TestSession(2)); + deferredCleanupQeue.add(new TestSession(2)); + + // the queue is full, so cleanup method should repeatedly call cleanup instead of queuing + TestSession session = new TestSession(5); + SessionManager.cleanup(deferredCleanupQeue, session); + assertEquals(-1, session.cleanupCount); + assertEquals(3, deferredCleanupQeue.size()); + assertTrue(deferredCleanupQeue.stream().allMatch(s -> ((TestSession) s).cleanupCount == 2)); + } + + @Test + public void testDefersCleanup() { + ArrayBlockingQueue deferredCleanupQeue = new ArrayBlockingQueue<>(3); + + deferredCleanupQeue.add(new TestSession(2)); + deferredCleanupQeue.add(new TestSession(2)); + + TestSession session = new TestSession(5); + + // the queue is not full so expect the session to be queued after cleanup + SessionManager.cleanup(deferredCleanupQeue, session); + + assertEquals(4, session.cleanupCount); + assertEquals(3, deferredCleanupQeue.size()); + assertEquals(2, + deferredCleanupQeue.stream().filter(s -> ((TestSession) s).cleanupCount == 2).count()); + assertEquals(1, + deferredCleanupQeue.stream().filter(s -> ((TestSession) s).cleanupCount == 4).count()); + } + + @Test + public void testDeferNotNeeded() { + ArrayBlockingQueue deferredCleanupQeue = new ArrayBlockingQueue<>(3); + + deferredCleanupQeue.add(new TestSession(2)); + deferredCleanupQeue.add(new TestSession(2)); + + TestSession session = new TestSession(0); + + // the queue is not full, but the session will cleanup in one call so it should not be queued + SessionManager.cleanup(deferredCleanupQeue, session); + + assertEquals(-1, session.cleanupCount); + assertEquals(2, deferredCleanupQeue.size()); + assertEquals(2, + deferredCleanupQeue.stream().filter(s -> ((TestSession) s).cleanupCount == 2).count()); + } +} From f3caedd2fe0591072f6f3384b18e98b0637fcd4b Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Mon, 10 Jul 2023 14:12:34 -0400 Subject: [PATCH 2/2] fixes build issue --- .../accumulo/tserver/session/SessionManagerTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java b/server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java index be0658dbca1..be3626ea712 100644 --- a/server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java +++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/session/SessionManagerTest.java @@ -19,11 +19,11 @@ package org.apache.accumulo.tserver.session; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.concurrent.ArrayBlockingQueue; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; public class SessionManagerTest { @@ -47,10 +47,10 @@ public boolean cleanup() { public void testTestcode() { // test behavior of test class TestSession session = new TestSession(2); - Assertions.assertFalse(session.cleanup()); - Assertions.assertFalse(session.cleanup()); - Assertions.assertTrue(session.cleanup()); - Assertions.assertTrue(session.cleanup()); + assertFalse(session.cleanup()); + assertFalse(session.cleanup()); + assertTrue(session.cleanup()); + assertTrue(session.cleanup()); } @Test