From 878294770d7cd90c0fd789f15e41d067a7c19deb Mon Sep 17 00:00:00 2001
From: Christopher Tubbs <ctubbsii@apache.org>
Date: Thu, 17 Mar 2022 18:00:00 -0400
Subject: [PATCH] Remove unneeded VisibleForTesting TabletServer stuff

* Replace unneeded copyClosedLogs method with built-in List.copyOf
* Replace ReferenceRemover with built-in Consumer
---
 .../apache/accumulo/tserver/TabletServer.java | 30 +++++--------------
 .../accumulo/tserver/WalRemovalOrderTest.java | 19 ++----------
 2 files changed, 10 insertions(+), 39 deletions(-)

diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
index f7a7af2bf62..3de818783a0 100644
--- a/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
+++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java
@@ -56,6 +56,7 @@
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.locks.ReentrantLock;
+import java.util.function.Consumer;
 
 import org.apache.accumulo.core.Constants;
 import org.apache.accumulo.core.client.Durability;
@@ -438,10 +439,10 @@ public void run() {
           sleepUninterruptibly(getConfiguration().getTimeInMillis(Property.TSERV_MAJC_DELAY),
               TimeUnit.MILLISECONDS);
 
-          List<DfsLogger> closedCopy;
+          final List<DfsLogger> closedCopy;
 
           synchronized (closedLogs) {
-            closedCopy = copyClosedLogs(closedLogs);
+            closedCopy = List.copyOf(closedLogs);
           }
 
           // bail early now if we're shutting down
@@ -1241,12 +1242,7 @@ public SecurityOperation getSecurityOperation() {
   // is used because its very import to know the order in which WALs were closed when deciding if a
   // WAL is eligible for removal. Maintaining the order that logs were used in is currently a simple
   // task because there is only one active log at a time.
-  LinkedHashSet<DfsLogger> closedLogs = new LinkedHashSet<>();
-
-  @VisibleForTesting
-  interface ReferencedRemover {
-    void removeInUse(Set<DfsLogger> candidates);
-  }
+  final LinkedHashSet<DfsLogger> closedLogs = new LinkedHashSet<>();
 
   /**
    * For a closed WAL to be eligible for removal it must be unreferenced AND all closed WALs older
@@ -1255,10 +1251,10 @@ interface ReferencedRemover {
    */
   @VisibleForTesting
   static Set<DfsLogger> findOldestUnreferencedWals(List<DfsLogger> closedLogs,
-      ReferencedRemover referencedRemover) {
+      Consumer<Set<DfsLogger>> referencedRemover) {
     LinkedHashSet<DfsLogger> unreferenced = new LinkedHashSet<>(closedLogs);
 
-    referencedRemover.removeInUse(unreferenced);
+    referencedRemover.accept(unreferenced);
 
     Iterator<DfsLogger> closedIter = closedLogs.iterator();
     Iterator<DfsLogger> unrefIter = unreferenced.iterator();
@@ -1279,25 +1275,15 @@ static Set<DfsLogger> findOldestUnreferencedWals(List<DfsLogger> closedLogs,
     return eligible;
   }
 
-  @VisibleForTesting
-  static List<DfsLogger> copyClosedLogs(LinkedHashSet<DfsLogger> closedLogs) {
-    List<DfsLogger> closedCopy = new ArrayList<>(closedLogs.size());
-    for (DfsLogger dfsLogger : closedLogs) {
-      // very important this copy maintains same order ..
-      closedCopy.add(dfsLogger);
-    }
-    return Collections.unmodifiableList(closedCopy);
-  }
-
   private void markUnusedWALs() {
 
     List<DfsLogger> closedCopy;
 
     synchronized (closedLogs) {
-      closedCopy = copyClosedLogs(closedLogs);
+      closedCopy = List.copyOf(closedLogs);
     }
 
-    ReferencedRemover refRemover = candidates -> {
+    Consumer<Set<DfsLogger>> refRemover = candidates -> {
       for (Tablet tablet : getOnlineTablets().values()) {
         tablet.removeInUseLogs(candidates);
         if (candidates.isEmpty()) {
diff --git a/server/tserver/src/test/java/org/apache/accumulo/tserver/WalRemovalOrderTest.java b/server/tserver/src/test/java/org/apache/accumulo/tserver/WalRemovalOrderTest.java
index ef8dac1c02d..8ccee731cd8 100644
--- a/server/tserver/src/test/java/org/apache/accumulo/tserver/WalRemovalOrderTest.java
+++ b/server/tserver/src/test/java/org/apache/accumulo/tserver/WalRemovalOrderTest.java
@@ -28,7 +28,6 @@
 import org.apache.accumulo.core.conf.AccumuloConfiguration;
 import org.apache.accumulo.core.conf.DefaultConfiguration;
 import org.apache.accumulo.server.fs.VolumeManager;
-import org.apache.accumulo.tserver.TabletServer.ReferencedRemover;
 import org.apache.accumulo.tserver.log.DfsLogger;
 import org.apache.accumulo.tserver.log.DfsLogger.ServerResources;
 import org.junit.jupiter.api.Test;
@@ -62,24 +61,10 @@ private static LinkedHashSet<DfsLogger> mockLoggers(String... logs) {
     return logSet;
   }
 
-  private static class TestRefRemover implements ReferencedRemover {
-    Set<DfsLogger> inUseLogs;
-
-    TestRefRemover(Set<DfsLogger> inUseLogs) {
-      this.inUseLogs = inUseLogs;
-    }
-
-    @Override
-    public void removeInUse(Set<DfsLogger> candidates) {
-      candidates.removeAll(inUseLogs);
-    }
-  }
-
   private static void runTest(LinkedHashSet<DfsLogger> closedLogs, Set<DfsLogger> inUseLogs,
       Set<DfsLogger> expected) {
-    List<DfsLogger> copy = TabletServer.copyClosedLogs(closedLogs);
-    Set<DfsLogger> eligible =
-        TabletServer.findOldestUnreferencedWals(copy, new TestRefRemover(inUseLogs));
+    Set<DfsLogger> eligible = TabletServer.findOldestUnreferencedWals(List.copyOf(closedLogs),
+        candidates -> candidates.removeAll(inUseLogs));
     assertEquals(expected, eligible);
   }