Skip to content

Use double‑checked locking to eliminate redundant metadata lookups #5538

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
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use double‑checked locking to eliminate redundant metadata lookups #5538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3fedaf
Use double‑checked locking to eliminate redundant metadata lookups
DomGarguilo May 6, 2025
0f00e0c
Use random singleton
DomGarguilo May 6, 2025
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
39 changes: 21 additions & 18 deletions core/src/main/java/org/apache/accumulo/core/clientImpl/ClientTabletCacheImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,25 +690,28 @@ private void lookupTablet(ClientContext context, Text row, LockCheckerSession lc
metadataRow.append(row.getBytes(), 0, row.getLength());
CachedTablet ptl = parent.findTablet(context, metadataRow, false, LocationNeed.REQUIRED);

if (ptl != null) {
// Only allow a single lookup at time per parent tablet. For example if a tables tablets are
// all stored in three metadata tablets, then that table could have up to three concurrent
// metadata lookups.
Timer timer = Timer.startNew();
try (var unused = lookupLocks.lock(ptl.getExtent())) {
// See if entry was added to cache by another thread while we were waiting on the lock
var cached = findTabletInCache(row);
if (cached != null && cached.getCreationTimer().startedAfter(timer)) {
Copy link
Contributor

@keith-turner keith-turner May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What made you suspect the timer here? I looked into the specifics of the timer and found how its implemented here https://github.com/openjdk/jdk/blob/b6b5ac1ef9042ed62a8358aa6943b8dc87dcf0ab/src/hotspot/os/posix/os_posix.cpp#L1474 . Looking at the docs for that system call it does mention that it will not go backwards but also there is no gaurantee it wil go forward between two calls. So its possible two threads got the exact same nanoTime causing the entry to look stale, that is probably ok. We could make the time comparison >= instead of > if that is the case.

Copy link
Contributor

@keith-turner keith-turner May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Changing the > to a >= is iffy and probably not good so please ignore that suggestion. One way we could avoid comparing the time or anything else is to just look for change in the object reference. So maybe could do the following?

   CachedTablet before = findTabletInCache(row);
  try (var unused = lookupLocks.lock(ptl.getExtent())) {
       CachedTablet after = findTabletInCache(row);
       if(after != before && after != null){
           // another thread probably did the lookup while we were waiting on the lock, so let use that
           return;
       }

// This cache entry was added after we started waiting on the lock so lets use it and not
// go to the metadata table. This means another thread was holding the lock and doing
// metadata lookups when we requested the lock.
return;
}
// Lookup tablets in metadata table and update cache. Also updating the cache while holding
// the lock is important as it ensures other threads that are waiting on the lock will see
// what this thread found and may be able to avoid metadata lookups.
lookupTablet(context, lcSession, ptl, metadataRow);
if (ptl == null) {
return;
}

CachedTablet now = findTabletInCache(row);
if (now != null) {
if (now.getTserverLocation().isPresent() && lcSession.checkLock(now) != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In support of ondemand tablets, tablets w/o a location can be cached in 4.0 (was not the case in 2.1). So these checks that look for the presence of a location do not seem correct.

return;
}
}
// Only allow a single lookup at time per parent tablet. For example if a tables tablets are
// all stored in three metadata tablets, then that table could have up to three concurrent
// metadata lookups.
try (var unused = lookupLocks.lock(ptl.getExtent())) {
now = findTabletInCache(row);
if (now != null && now.getTserverLocation().isPresent() && lcSession.checkLock(now) != null) {
return;
}
// Lookup tablets in metadata table and update cache. Also updating the cache while holding
// the lock is important as it ensures other threads that are waiting on the lock will see
// what this thread found and may be able to avoid metadata lookups.
lookupTablet(context, lcSession, ptl, metadataRow);
}
}

Expand Down
62 changes: 37 additions & 25 deletions core/src/test/java/org/apache/accumulo/core/clientImpl/ClientTabletCacheImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.accumulo.core.clientImpl;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.apache.accumulo.core.util.LazySingletons.RANDOM;
import static org.easymock.EasyMock.replay;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
Expand Down Expand Up @@ -1986,45 +1987,56 @@ public void testMultithreadedLookups() throws Exception {
setLocation(tservers, "tserver3", mte2, ke3, "tserver9");

var executor = Executors.newCachedThreadPool();
List<Future<CachedTablet>> futures = new ArrayList<>();
final int lookupCount = 128;
final int roundCount = 8;

// start 64 threads all trying to lookup data in the cache, should see only two threads do a
// concurrent lookup in the metadata table and no more or less.
List<String> rowsToLookup = new ArrayList<>();
List<Future<CachedTablet>> futures = new ArrayList<>(roundCount * lookupCount);

for (int i = 0; i < 64; i++) {
String lookup = (char) ('a' + (i % 26)) + "";
rowsToLookup.add(lookup);
}
// multiple rounds to increase the chance of contention
for (int round = 0; round < roundCount; round++) {

Collections.shuffle(rowsToLookup);

for (var lookup : rowsToLookup) {
var future = executor.submit(() -> {
var loc = metaCache.findTablet(context, new Text(lookup), false, LocationNeed.REQUIRED);
if (lookup.compareTo("m") <= 0) {
assertEquals("tserver7", loc.getTserverLocation().orElseThrow());
} else if (lookup.compareTo("q") <= 0) {
assertEquals("tserver8", loc.getTserverLocation().orElseThrow());
} else {
assertEquals("tserver9", loc.getTserverLocation().orElseThrow());
}
return loc;
});
futures.add(future);
// start a bunch of threads all trying to lookup data in the cache
// should see exactly 2 threads doing metadata lookups at a time
List<String> rowsToLookup = new ArrayList<>(lookupCount);

for (int i = 0; i < lookupCount; i++) {
String lookup = (char) ('a' + (i % 26)) + "";
rowsToLookup.add(lookup);
}

Collections.shuffle(rowsToLookup, RANDOM.get());

for (var lookup : rowsToLookup) {
var future = executor.submit(() -> {
if (RANDOM.get().nextInt(10) < 3) {
Thread.yield();
}
var loc = metaCache.findTablet(context, new Text(lookup), false, LocationNeed.REQUIRED);
if (lookup.compareTo("m") <= 0) {
assertEquals("tserver7", loc.getTserverLocation().orElseThrow());
} else if (lookup.compareTo("q") <= 0) {
assertEquals("tserver8", loc.getTserverLocation().orElseThrow());
} else {
assertEquals("tserver9", loc.getTserverLocation().orElseThrow());
}
return loc;
});
futures.add(future);
}
}

for (var future : futures) {
assertNotNull(future.get());
}

assertTrue(sawTwoActive.get());
assertTrue(sawTwoActive.get(), "Expected to see exactly two lookups.");
// The second metadata tablet (mte2) contains two user tablets (ke2 and ke3). Depending on which
// of these two user tablets is looked up in the metadata table first will see a total of 2 or 3
// lookups. If the location of ke2 is looked up first then it will get the locations of ke2 and
// ke3 from mte2 and put them in the cache. If the location of ke3 is looked up first then it
// will only get the location of ke3 from mte2 and not ke2.
assertTrue(lookups.size() == 2 || lookups.size() == 3, lookups::toString);
assertTrue(lookups.size() == 2 || lookups.size() == 3,
"Expected 2 or 3 lookups, got " + lookups.size() + " : " + lookups);
assertEquals(1, lookups.stream().filter(metadataExtent -> metadataExtent.equals(mte1)).count(),
lookups::toString);
var mte2Lookups =
Expand Down