Skip to content

Commit f04e5f3

Browse files
committed
[fix](fe) Publish metadata IDs with cache objects
### What problem does this PR solve? Issue Number: close #64159 Related PR: #64160 Problem Summary: Incremental metadata events published the ID-to-name mapping before the matching database or table object was committed to the shared cache. A concurrent lookup by ID could therefore load and publish a different object in that gap, which was then replaced and cleaned up by the event update. Publish the ID mapping and object inside one guarded cache commit, fence overlapping loads, and make the object-and-ID step run only once when the outer name-cache CAS retries so a newer object is never replaced by replaying an already completed event update. ### Release note None ### Check List (For Author) - Test: Unit Test - ScopedMetaCacheConcurrencyTest - FeMetaCacheEntryTest - ExternalCatalogTest - ExternalDatabaseTest - ./build.sh --fe - Behavior changed: No - Does this need documentation: No
1 parent 5c0c857 commit f04e5f3

7 files changed

Lines changed: 277 additions & 11 deletions

File tree

fe/fe-connector/fe-connector-cache/src/main/java/org/apache/doris/connector/cache/MetaCache.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,21 @@ public void put(K key, V value) {
7272
}
7373

7474
public boolean compareAndSet(K key, V expectedValue, V updatedValue) {
75+
return compareAndSet(key, expectedValue, updatedValue, () -> {
76+
});
77+
}
78+
79+
/**
80+
* Updates a value only if it is still current, and runs an auxiliary action inside the same guarded commit.
81+
* Concurrent cache loads can not publish between the action and the value update.
82+
* The action runs while the publication guards and key lock are held, so it must be short, non-blocking, and
83+
* must not re-enter this cache. If it throws, the value is not updated, but completed external side effects are
84+
* not rolled back.
85+
*/
86+
public boolean compareAndSet(K key, V expectedValue, V updatedValue, Runnable commitAction) {
7587
K nonNullKey = Objects.requireNonNull(key, "key can not be null");
76-
return delegate.compareAndSet(nonNullKey, definition.scope(nonNullKey), expectedValue, updatedValue);
88+
return delegate.compareAndSet(nonNullKey, definition.scope(nonNullKey), expectedValue, updatedValue,
89+
Objects.requireNonNull(commitAction, "commitAction can not be null"));
7790
}
7891

7992
public void invalidateKey(K key) {

fe/fe-connector/fe-connector-cache/src/main/java/org/apache/doris/connector/cache/ScopedMetaCache.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,10 +292,17 @@ public void put(K key, ScopePath path, V value) {
292292
}
293293

294294
public boolean compareAndSet(K key, ScopePath path, V expectedValue, V updatedValue) {
295+
return compareAndSet(key, path, expectedValue, updatedValue, NO_OP);
296+
}
297+
298+
public boolean compareAndSet(
299+
K key, ScopePath path, V expectedValue, V updatedValue, Runnable commitAction) {
295300
Objects.requireNonNull(key, "key can not be null");
296301
Objects.requireNonNull(path, "path can not be null");
302+
Runnable action = Objects.requireNonNull(commitAction, "commitAction can not be null");
297303
checkOpen();
298304
if (!effectiveEnabled) {
305+
action.run();
299306
return true;
300307
}
301308
try (PublicationLease<K, V> lease = acquirePublicationLease(key, path, false)) {
@@ -306,7 +313,9 @@ public boolean compareAndSet(K key, ScopePath path, V expectedValue, V updatedVa
306313
return false;
307314
}
308315
lease.keyNode.loadPublicationState.set(new Object());
316+
action.run();
309317
if (updatedValue == currentValue) {
318+
lease.keyNode.loadPublicationState.set(new Object());
310319
return true;
311320
}
312321
if (updatedValue == null) {
@@ -317,6 +326,7 @@ public boolean compareAndSet(K key, ScopePath path, V expectedValue, V updatedVa
317326
} else {
318327
publishCommitted(lease, key, updatedValue);
319328
}
329+
lease.keyNode.loadPublicationState.set(new Object());
320330
return true;
321331
});
322332
}

fe/fe-connector/fe-connector-cache/src/test/java/org/apache/doris/connector/cache/ScopedMetaCacheConcurrencyTest.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,37 @@ public void concurrentMissesForTheSameKeyRunOneLoader() throws Exception {
7171
}
7272
}
7373

74+
@Test
75+
public void commitActionPreventsConcurrentMissFromPublishingBetweenIndexAndValue() throws Exception {
76+
ExecutorService executor = Executors.newFixedThreadPool(2);
77+
CountDownLatch actionStarted = new CountDownLatch(1);
78+
CountDownLatch concurrentLoadElected = new CountDownLatch(1);
79+
AtomicInteger loads = new AtomicInteger();
80+
try (ScopedMetaCacheRegistry registry = new ScopedMetaCacheRegistry()) {
81+
ScopedMetaCache<String, String> cache = registry.createCache(
82+
"test", ENABLED, null, null, concurrentLoadElected::countDown, () -> {
83+
});
84+
85+
Future<Boolean> update = executor.submit(() -> cache.compareAndSet(
86+
"key", TABLE, null, "event-value", () -> {
87+
actionStarted.countDown();
88+
await(concurrentLoadElected);
89+
}));
90+
await(actionStarted);
91+
Future<String> lookup = executor.submit(() -> cache.get("key", TABLE, ignored -> {
92+
loads.incrementAndGet();
93+
return "lookup-value";
94+
}));
95+
96+
Assertions.assertTrue(update.get(TIMEOUT_SECONDS, TimeUnit.SECONDS));
97+
Assertions.assertEquals("event-value", lookup.get(TIMEOUT_SECONDS, TimeUnit.SECONDS));
98+
Assertions.assertEquals("event-value", cache.getIfPresent("key", TABLE));
99+
Assertions.assertEquals(0, loads.get());
100+
} finally {
101+
executor.shutdownNow();
102+
}
103+
}
104+
74105
@Test
75106
public void closeDuringRefreshAdmissionDoesNotRetainRefreshMarker() throws Exception {
76107
ExecutorService executor = Executors.newSingleThreadExecutor();

fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@
8080
import java.util.Optional;
8181
import java.util.Set;
8282
import java.util.concurrent.ThreadPoolExecutor;
83+
import java.util.concurrent.atomic.AtomicBoolean;
8384
import java.util.function.Function;
8485
import java.util.stream.Collectors;
8586

@@ -1506,6 +1507,7 @@ private void updateDatabaseCache(String remoteDbName, String localDbName,
15061507
// lightweight lookup index and must always track registered objects so normal by-ID lookup can load on demand.
15071508
// By default, incremental updates keep cold names/object cache entries cold.
15081509
// forceUpdateCacheState is only for paths that intentionally populate those cold entries.
1510+
AtomicBoolean objectEntryUpdated = new AtomicBoolean();
15091511
databaseNames.computeAfterValidation(
15101512
"",
15111513
(ignored, current) -> {
@@ -1517,10 +1519,18 @@ private void updateDatabaseCache(String remoteDbName, String localDbName,
15171519
// can not publish a stale snapshot after this incremental update.
15181520
return current == null ? null : current.withName(remoteDbName, localDbName);
15191521
},
1520-
() -> databases.computeAfterValidation(
1521-
localDbName,
1522-
(ignored, current) -> (forceUpdateCacheState || current != null) ? db : null,
1523-
() -> dbIdNameIndex.put(dbId, localDbName)));
1522+
() -> {
1523+
// The outer names CAS may retry after this object step succeeds. Do not replay the object
1524+
// ownership change: a later lookup may already have published and returned a newer object.
1525+
if (objectEntryUpdated.get()) {
1526+
return;
1527+
}
1528+
databases.computeWithCommitAction(
1529+
localDbName,
1530+
(ignored, current) -> (forceUpdateCacheState || current != null) ? db : null,
1531+
() -> dbIdNameIndex.put(dbId, localDbName));
1532+
objectEntryUpdated.set(true);
1533+
});
15241534
}
15251535

15261536
protected void invalidateDatabaseCache(String localDbName) {

fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@
5959
import java.util.Optional;
6060
import java.util.Set;
6161
import java.util.concurrent.TimeUnit;
62+
import java.util.concurrent.atomic.AtomicBoolean;
6263
import java.util.function.Function;
6364
import java.util.stream.Collectors;
6465

@@ -701,6 +702,7 @@ protected void updateTableCache(T table, String remoteTableName, String localTab
701702
// lightweight lookup index and must always track registered objects so normal by-ID lookup can load on demand.
702703
// By default, incremental updates keep cold names/object cache entries cold.
703704
// forceUpdateCacheState is only for paths that intentionally populate those cold entries.
705+
AtomicBoolean objectEntryUpdated = new AtomicBoolean();
704706
tableNames.computeAfterValidation(
705707
"",
706708
(ignored, current) -> {
@@ -712,10 +714,18 @@ protected void updateTableCache(T table, String remoteTableName, String localTab
712714
// can not publish a stale snapshot after this incremental update.
713715
return current == null ? null : current.withName(remoteTableName, localTableName);
714716
},
715-
() -> tables.computeAfterValidation(
716-
localTableName,
717-
(ignored, current) -> (forceUpdateCacheState || current != null) ? table : null,
718-
() -> tableIdNameIndex.put(table.getId(), localTableName)));
717+
() -> {
718+
// The outer names CAS may retry after this object step succeeds. Do not replay the object
719+
// ownership change: a later lookup may already have published a newer object.
720+
if (objectEntryUpdated.get()) {
721+
return;
722+
}
723+
tables.computeWithCommitAction(
724+
localTableName,
725+
(ignored, current) -> (forceUpdateCacheState || current != null) ? table : null,
726+
() -> tableIdNameIndex.put(table.getId(), localTableName));
727+
objectEntryUpdated.set(true);
728+
});
719729
}
720730

721731
protected void invalidateTableCache(String localTableName) {

fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/FeMetaCacheEntry.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,29 @@ public V computeAfterValidation(K key, BiFunction<K, V, V> remappingFunction, Ru
320320
}
321321
}
322322

323+
/**
324+
* Runs the action only after the expected cache value is confirmed, while concurrent loads are prevented from
325+
* publishing until both the action and the value update finish. The action runs while this entry's stripe and
326+
* the shared-cache publication locks are held, so it must be short, non-blocking, and must not re-enter this
327+
* entry. If it throws, the value is not updated, but completed external side effects are not rolled back.
328+
*/
329+
public V computeWithCommitAction(K key, BiFunction<K, V, V> remappingFunction, Runnable commitAction) {
330+
BiFunction<K, V, V> remapper = Objects.requireNonNull(remappingFunction, "remappingFunction can not be null");
331+
Runnable action = Objects.requireNonNull(commitAction, "commitAction can not be null");
332+
StripeState<K> stripe = stripeState(key);
333+
synchronized (stripe) {
334+
while (true) {
335+
V current = data.getIfPresent(key);
336+
V updated = effectiveEnabled ? remapper.apply(key, current) : null;
337+
bumpAction(stripe, key);
338+
beforePublicMutationWriteForTest(key);
339+
if (data.compareAndSet(key, current, updated, action)) {
340+
return updated;
341+
}
342+
}
343+
}
344+
}
345+
323346
public void invalidateKey(K key) {
324347
invalidateKeyAndRun(key, () -> {
325348
});

0 commit comments

Comments
 (0)