Skip to content

Remove unused extents from tablet locator #6498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DomGarguilo wants to merge 1 commit into
base: 2.1
Choose a base branch
from
+36 −2
Open

Remove unused extents from tablet locator #6498

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static org.apache.accumulo.core.util.UtilWaitThread.sleepUninterruptibly;

import java.time.Duration;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -54,11 +55,17 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.RemovalCause;
import com.github.benmanes.caffeine.cache.Scheduler;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

public class TabletLocatorImpl extends TabletLocator {

private static final Logger log = LoggerFactory.getLogger(TabletLocatorImpl.class);
private static final Duration CACHE_EXPIRATION = Duration.ofMinutes(10);

// MAX_TEXT represents a TEXT object that is greater than all others. Attempted to use null for
// this purpose, but there seems to be a bug in TreeMap.tailMap with null. Therefore instead of
Expand All @@ -80,7 +87,9 @@ public class TabletLocatorImpl extends TabletLocator {

protected TableId tableId;
protected TabletLocator parent;
// The TreeMap supports range lookups; Caffeine tracks access and expires entries from it.
protected TreeMap<Text,TabletLocation> metaCache = new TreeMap<>(END_ROW_COMPARATOR);
private final Cache<KeyExtent,TabletLocation> extentCache;
protected TabletLocationObtainer locationObtainer;
private final TabletServerLockChecker lockChecker;
protected Text lastTabletRow;
Expand Down Expand Up @@ -158,10 +167,29 @@ public TabletLocatorImpl(TableId tableId, TabletLocator parent, TabletLocationOb
this.locationObtainer = tlo;
this.lockChecker = tslc;

extentCache = Caffeine.newBuilder().expireAfterAccess(CACHE_EXPIRATION)
.scheduler(Scheduler.systemScheduler()).removalListener(this::onExtentRemoval).build();

this.lastTabletRow = new Text(tableId.canonical());
lastTabletRow.append(new byte[] {'<'}, 0, 1);
}

private void onExtentRemoval(KeyExtent extent, TabletLocation location, RemovalCause cause) {

@dlmarion dlmarion Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be invoked asynchronously which could allow the extent to be accessed after an eviction decision has already been made. Looking at the docs, I wonder if we should use evictionListener instead of removalListener on the cache for synchronous notification.

if (cause == RemovalCause.REPLACED) {
return;
}

wLock.lock();
try {
Text endRow = extent.endRow() == null ? MAX_TEXT : extent.endRow();
if (metaCache.get(endRow) == location) {
metaCache.remove(endRow);
}
} finally {
wLock.unlock();
}
}

@Override
public <T extends Mutation> void binMutations(ClientContext context, List<T> mutations,
Map<String,TabletServerMutations<T>> binnedMutations, List<T> failures)
Expand Down Expand Up @@ -465,6 +493,7 @@ public void invalidateCache() {
try {
invalidatedCount = metaCache.size();
metaCache.clear();
extentCache.invalidateAll();
} finally {
wLock.unlock();
}
Expand Down Expand Up @@ -596,6 +625,7 @@ private void updateCache(TabletLocation tabletLocation, LockCheckerSession lcSes
er = MAX_TEXT;
}
metaCache.put(er, tabletLocation);
extentCache.put(tabletLocation.tablet_extent, tabletLocation);

if (!badExtents.isEmpty()) {
removeOverlapping(badExtents, tabletLocation.tablet_extent);
Expand Down Expand Up @@ -648,9 +678,13 @@ private TabletLocation locateTabletInCache(Text row) {
Entry<Text,TabletLocation> entry = metaCache.ceilingEntry(row);

if (entry != null) {
KeyExtent ke = entry.getValue().tablet_extent;
TabletLocation location = extentCache.getIfPresent(entry.getValue().tablet_extent);

@dlmarion dlmarion Aug 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code may not need to change if instead we synchronize on an object in this method and in the evictionListener to ensure that an eviction and lookup are not happening concurrently. I have not looked at other places in this class where extentCache lookups are done, but we would want to protect those as well.

Could use Guava's Striped class,Striped<Lock> specifically, for more granular locking on the extent.

if (location == null) {
return null;
}
KeyExtent ke = location.tablet_extent;
if (ke.prevEndRow() == null || ke.prevEndRow().compareTo(row) < 0) {
return entry.getValue();
return location;
}
}
return null;
Expand Down