From 7fe767ead59d59dabd80139647261149fb15a519 Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Fri, 6 Mar 2026 21:47:29 +0000 Subject: [PATCH 1/2] Clears unreferenced tables from TabletLocator fixes #6164 --- .../core/clientImpl/TabletLocator.java | 70 ++++++++++++++-- .../org/apache/accumulo/test/LocatorIT.java | 80 +++++++++++++++++++ 2 files changed, 145 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java index 353bfd6da04..63bd78a1321 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkArgument; +import java.time.Duration; import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; @@ -41,8 +42,10 @@ import org.apache.accumulo.core.singletons.SingletonManager; import org.apache.accumulo.core.singletons.SingletonService; import org.apache.accumulo.core.util.Interner; +import org.apache.accumulo.core.util.Timer; import org.apache.hadoop.io.Text; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; public abstract class TabletLocator { @@ -111,7 +114,8 @@ public boolean equals(LocatorKey lk) { } private static final HashMap locators = new HashMap<>(); - private static final HashMap offlineLocators = new HashMap<>(); + private static final HashMap offlineLocators = + new HashMap<>(); private static boolean enabled = true; public static synchronized void clearLocators() { @@ -138,15 +142,17 @@ static synchronized void enable() { public static synchronized TabletLocator getLocator(ClientContext context, TableId tableId) { Preconditions.checkState(enabled, "The Accumulo singleton that that tracks tablet locations is " + "disabled. This is likely caused by all AccumuloClients being closed or garbage collected"); + + clearUnusedTables(context); + TableState state = context.getTableState(tableId); + LocatorKey key = new LocatorKey(context.getInstanceID(), tableId); if (state == TableState.OFFLINE) { - LocatorKey key = new LocatorKey(context.getInstanceID(), tableId); locators.remove(key); - return offlineLocators.computeIfAbsent(tableId, + return offlineLocators.computeIfAbsent(key, f -> new OfflineTabletLocatorImpl(context, tableId)); } else { - offlineLocators.remove(tableId); - LocatorKey key = new LocatorKey(context.getInstanceID(), tableId); + offlineLocators.remove(key); TabletLocator tl = locators.get(key); if (tl == null) { MetadataLocationObtainer mlo = new MetadataLocationObtainer(); @@ -167,6 +173,60 @@ public static synchronized TabletLocator getLocator(ClientContext context, Table } + /** + * Checks if a table id is present in the cache w/o creating it. + */ + @VisibleForTesting + public static synchronized boolean isPresent(ClientContext context, TableId tableId) { + LocatorKey key = new LocatorKey(context.getInstanceID(), tableId); + return locators.containsKey(key) || offlineLocators.containsKey(key); + } + + private static Duration clearFrequency = Duration.ofMinutes(10); + + /** + * Sets how often checks for unused tables are done + */ + @VisibleForTesting + public static synchronized void setClearFrequency(Duration frequency) { + Preconditions.checkArgument(frequency != null && !frequency.isNegative() && !frequency.isZero(), + "frequency:%s", frequency); + clearFrequency = frequency; + } + + /** + * Finds and clears any tables ids in the cache that are no longer in used. + */ + private static final Timer lastClearTimer = Timer.startNew(); + + private static synchronized void clearUnusedTables(ClientContext context) { + if (lastClearTimer.hasElapsed(clearFrequency)) { + locators.entrySet().removeIf(entry -> { + LocatorKey lkey = entry.getKey(); + TabletLocator locator = entry.getValue(); + if (lkey.instanceId.equals(context.getInstanceID()) + && context.getTableState(lkey.tableId) != TableState.ONLINE) { + locator.isValid = false; + locator.invalidateCache(); + return true; + } + return false; + }); + offlineLocators.entrySet().removeIf(entry -> { + LocatorKey lkey = entry.getKey(); + TabletLocator locator = entry.getValue(); + if (lkey.instanceId.equals(context.getInstanceID()) + && context.getTableState(lkey.tableId) != TableState.OFFLINE) { + locator.isValid = false; + locator.invalidateCache(); + return true; + } + return false; + }); + lastClearTimer.restart(); + } + } + static { SingletonManager.register(new SingletonService() { diff --git a/test/src/main/java/org/apache/accumulo/test/LocatorIT.java b/test/src/main/java/org/apache/accumulo/test/LocatorIT.java index 289a6f85574..c51af1e0b58 100644 --- a/test/src/main/java/org/apache/accumulo/test/LocatorIT.java +++ b/test/src/main/java/org/apache/accumulo/test/LocatorIT.java @@ -19,6 +19,7 @@ package org.apache.accumulo.test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -38,13 +39,18 @@ import org.apache.accumulo.core.client.TableNotFoundException; import org.apache.accumulo.core.client.TableOfflineException; import org.apache.accumulo.core.client.admin.Locations; +import org.apache.accumulo.core.client.admin.NewTableConfiguration; import org.apache.accumulo.core.client.admin.TableOperations; +import org.apache.accumulo.core.clientImpl.ClientContext; +import org.apache.accumulo.core.clientImpl.TabletLocator; import org.apache.accumulo.core.data.Range; import org.apache.accumulo.core.data.TableId; import org.apache.accumulo.core.data.TabletId; import org.apache.accumulo.core.dataImpl.KeyExtent; import org.apache.accumulo.core.dataImpl.TabletIdImpl; +import org.apache.accumulo.core.manager.state.tables.TableState; import org.apache.accumulo.harness.AccumuloClusterHarness; +import org.apache.accumulo.test.util.Wait; import org.apache.hadoop.io.Text; import org.junit.jupiter.api.Test; @@ -132,4 +138,78 @@ public void testBasic() throws Exception { assertThrows(TableNotFoundException.class, () -> tableOps.locate(tableName, ranges)); } } + + @Test + public void testClearingUnused() throws Exception { + try (AccumuloClient client = Accumulo.newClient().from(getClientProps()).build()) { + String[] tables = getUniqueNames(4); + String table1 = tables[0]; + String table2 = tables[1]; + String table3 = tables[2]; + String table4 = tables[3]; + + TableOperations tableOps = client.tableOperations(); + tableOps.create(table1); + tableOps.create(table2); + tableOps.create(table3, new NewTableConfiguration().createOffline()); + tableOps.create(table4, new NewTableConfiguration().createOffline()); + + TabletLocator.setClearFrequency(Duration.ofMillis(100)); + + ClientContext ctx = (ClientContext) client; + TableId tableId1 = ctx.getTableId(table1); + TableId tableId2 = ctx.getTableId(table2); + TableId tableId3 = ctx.getTableId(table3); + TableId tableId4 = ctx.getTableId(table4); + + for (var tableId : List.of(tableId1, tableId2, tableId3, tableId4)) { + assertFalse(TabletLocator.isPresent(ctx, tableId)); + assertNotNull(TabletLocator.getLocator(ctx, tableId)); + assertTrue(TabletLocator.isPresent(ctx, tableId)); + } + + // Put table2 and table3 into a different state than what is in the cache + assertEquals(TableState.ONLINE, ctx.getTableState(tableId2)); + assertEquals(TableState.OFFLINE, ctx.getTableState(tableId3)); + tableOps.offline(table2, true); + tableOps.online(table3, true); + assertEquals(TableState.OFFLINE, ctx.getTableState(tableId2)); + assertEquals(TableState.ONLINE, ctx.getTableState(tableId3)); + + Wait.waitFor(() -> { + // Accessing table1 in the cache should cause table2 and table3 to eventually be cleared + // because their table state does not match what was cached + assertNotNull(TabletLocator.getLocator(ctx, tableId1)); + return !TabletLocator.isPresent(ctx, tableId2) && !TabletLocator.isPresent(ctx, tableId3); + }); + + assertTrue(TabletLocator.isPresent(ctx, tableId1)); + assertTrue(TabletLocator.isPresent(ctx, tableId4)); + + // bring table2 and table3 back into the cache + for (var tableId : List.of(tableId2, tableId3)) { + assertFalse(TabletLocator.isPresent(ctx, tableId)); + assertNotNull(TabletLocator.getLocator(ctx, tableId)); + assertTrue(TabletLocator.isPresent(ctx, tableId)); + } + + tableOps.delete(table2); + tableOps.delete(table3); + + Wait.waitFor(() -> { + // Accessing table4 in the cache should cause table2 and table3 to eventually be cleared + // because they no longer exist. This also test that online and offline tables a properly + // cleared from the cache. + assertNotNull(TabletLocator.getLocator(ctx, tableId4)); + return !TabletLocator.isPresent(ctx, tableId2) && !TabletLocator.isPresent(ctx, tableId3); + }); + + // table1 and table4 should be left in the cache, check that online or offline tables are not + // removed unnecessarily. + assertTrue(TabletLocator.isPresent(ctx, tableId1)); + assertTrue(TabletLocator.isPresent(ctx, tableId4)); + assertEquals(TableState.ONLINE, ctx.getTableState(tableId1)); + assertEquals(TableState.OFFLINE, ctx.getTableState(tableId4)); + } + } } From 14fb90b2a678f96617b0c63929cd46c895c2e99a Mon Sep 17 00:00:00 2001 From: Keith Turner Date: Fri, 6 Mar 2026 21:52:55 +0000 Subject: [PATCH 2/2] move comment to correct place --- .../org/apache/accumulo/core/clientImpl/TabletLocator.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java b/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java index 63bd78a1321..6375b9cf841 100644 --- a/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java +++ b/core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java @@ -194,11 +194,11 @@ public static synchronized void setClearFrequency(Duration frequency) { clearFrequency = frequency; } + private static final Timer lastClearTimer = Timer.startNew(); + /** * Finds and clears any tables ids in the cache that are no longer in used. */ - private static final Timer lastClearTimer = Timer.startNew(); - private static synchronized void clearUnusedTables(ClientContext context) { if (lastClearTimer.hasElapsed(clearFrequency)) { locators.entrySet().removeIf(entry -> {