Skip to content

Commit 54cac4e

Browse files
committed
feat: Drop persistent-store cache after FDv2 in-memory store init
Once the FDv2 in-memory store takes over as the active read source, the persistent-store wrapper's three Guava caches are no longer consulted on reads and roughly double the in-memory footprint of flag data (or worse, indefinitely, in cacheForever() mode). Add an internal DisableableCache capability interface implemented by PersistentDataStoreWrapper. The wrapper's disableCache() sets a volatile cacheDisabled flag and invalidates all three Guava caches. Every cache touch site (isInitialized, init, get, getAll, upsert, getCacheStats, pollAvailabilityAfterOutage) checks the flag and short-circuits to the core when set; this is required because the caches are LoadingCache instances and plain invalidation would auto-repopulate via the registered CacheLoaders on the next get(). WriteThroughStore.maybeSwitchStore() probes for the interface and invokes disableCache() inside the existing synchronized block, after the active read store has been flipped to the in-memory store. The PersistentDataStoreBuilder cache-config setters (cacheTime, cacheSeconds, cacheMillis, cacheForever, noCaching, staleValuesPolicy, recordCacheStats) remain functional during the bootstrap window for backward compatibility; class-level javadoc explains that under FDv2 they only govern that window. Naming note: the capability is named disableCache rather than the ticket's clearCache so the verb conveys that the cache is off going forward, not just emptied. This aligns with Python and Ruby's disable_cache and matches the dotnet sibling PR. Mirrors dotnet-core#274 (.NET), launchdarkly/python-server-sdk#426 (Python), launchdarkly/ruby-server-sdk#384 (Ruby), and launchdarkly/go-server-sdk#373 (Go).
1 parent cac1568 commit 54cac4e

6 files changed

Lines changed: 275 additions & 12 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package com.launchdarkly.sdk.server;
2+
3+
/**
4+
* Optional interface for data stores that can disable their internal cache.
5+
* <p>
6+
* This is currently for internal implementations only.
7+
*/
8+
interface DisableableCache {
9+
/**
10+
* Disables the internal cache. After this call, the cache is no longer
11+
* consulted on reads and no longer populated by writes.
12+
* <p>
13+
* Implementations should release the cache contents so the memory can be
14+
* reclaimed. The call must be idempotent: subsequent invocations should be
15+
* safe and have no further effect.
16+
*/
17+
void disableCache();
18+
}

lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/PersistentDataStoreWrapper.java

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343
* <p>
4444
* This class is only constructed by {@link PersistentDataStoreBuilder}.
4545
*/
46-
final class PersistentDataStoreWrapper implements DataStore, SettableCache {
46+
final class PersistentDataStoreWrapper implements DataStore, SettableCache, DisableableCache {
4747
private final PersistentDataStore core;
4848
private final LoadingCache<CacheKey, Optional<ItemDescriptor>> itemCache;
4949
private final LoadingCache<DataKind, KeyedItems<ItemDescriptor>> allCache;
@@ -54,9 +54,15 @@ final class PersistentDataStoreWrapper implements DataStore, SettableCache {
5454
private final AtomicBoolean inited = new AtomicBoolean(false);
5555
private final ListeningExecutorService cacheExecutor;
5656
private final LDLogger logger;
57-
57+
5858
private final Object externalStoreLock = new Object();
5959
private volatile CacheExporter externalCache;
60+
61+
// Once true, the cache is bypassed on reads and writes; entries already in
62+
// the cache have been invalidated by disableCache(). The cache instances
63+
// themselves remain alive until GC reclaims them; the LoadingCache loaders
64+
// are short-circuited because every touch site checks this flag first.
65+
private volatile boolean cacheDisabled;
6066

6167
PersistentDataStoreWrapper(
6268
final PersistentDataStore core,
@@ -151,14 +157,31 @@ public void close() throws IOException {
151157
core.close();
152158
}
153159

160+
@Override
161+
public void disableCache() {
162+
if (cacheDisabled) return;
163+
// Volatile write publishes the bypass flag before clearing cache contents.
164+
// Future readers observe cacheDisabled == true and skip the cache call
165+
// sites; an in-flight reader past the flag check may still complete its
166+
// current cache operation and, on a miss, fire the LoadingCache loader
167+
// which can repopulate. Those leftover entries hold fresh values and are
168+
// unreachable from any subsequent read (every future caller bypasses), so
169+
// they don't cause stale reads or torn observations; they simply persist
170+
// until GC reclaims the cache. See dotnet-core#274 for the analysis.
171+
cacheDisabled = true;
172+
if (itemCache != null) itemCache.invalidateAll();
173+
if (allCache != null) allCache.invalidateAll();
174+
if (initCache != null) initCache.invalidateAll();
175+
}
176+
154177
@Override
155178
public boolean isInitialized() {
156179
if (inited.get()) {
157180
return true;
158181
}
159182
boolean result;
160183
try {
161-
if (initCache != null) {
184+
if (initCache != null && !cacheDisabled) {
162185
result = initCache.get("");
163186
} else {
164187
result = core.isInitialized();
@@ -187,7 +210,7 @@ public void init(FullDataSet<ItemDescriptor> allData) {
187210
allBuilder.add(new AbstractMap.SimpleEntry<>(kind, items));
188211
}
189212
RuntimeException failure = initCore(new FullDataSet<>(allBuilder.build(), allData.shouldPersist()));
190-
if (itemCache != null && allCache != null) {
213+
if (itemCache != null && allCache != null && !cacheDisabled) {
191214
itemCache.invalidateAll();
192215
allCache.invalidateAll();
193216
if (failure != null && !cacheIndefinitely) {
@@ -228,7 +251,7 @@ private RuntimeException initCore(FullDataSet<SerializedItemDescriptor> allData)
228251
@Override
229252
public ItemDescriptor get(DataKind kind, String key) {
230253
try {
231-
ItemDescriptor ret = itemCache != null ? itemCache.get(CacheKey.forItem(kind, key)).orNull() :
254+
ItemDescriptor ret = (itemCache != null && !cacheDisabled) ? itemCache.get(CacheKey.forItem(kind, key)).orNull() :
232255
getAndDeserializeItem(kind, key);
233256
processError(null);
234257
return ret;
@@ -242,7 +265,7 @@ public ItemDescriptor get(DataKind kind, String key) {
242265
public KeyedItems<ItemDescriptor> getAll(DataKind kind) {
243266
try {
244267
KeyedItems<ItemDescriptor> ret;
245-
ret = allCache != null ? allCache.get(kind) : getAllAndDeserialize(kind);
268+
ret = (allCache != null && !cacheDisabled) ? allCache.get(kind) : getAllAndDeserialize(kind);
246269
processError(null);
247270
return ret;
248271
} catch (Exception e) {
@@ -281,7 +304,7 @@ public boolean upsert(DataKind kind, String key, ItemDescriptor item) {
281304
}
282305
failure = e;
283306
}
284-
if (itemCache != null) {
307+
if (itemCache != null && !cacheDisabled) {
285308
CacheKey cacheKey = CacheKey.forItem(kind, key);
286309
if (failure == null) {
287310
if (updated) {
@@ -297,7 +320,7 @@ public boolean upsert(DataKind kind, String key, ItemDescriptor item) {
297320
}
298321
}
299322
}
300-
if (allCache != null) {
323+
if (allCache != null && !cacheDisabled) {
301324
// If the cache has a finite TTL, then we should remove the "all items" cache entry to force
302325
// a reread the next time All is called. However, if it's an infinite TTL, we need to just
303326
// update the item within the existing "all items" entry (since we want things to still work
@@ -340,7 +363,7 @@ public void setCacheExporter(CacheExporter externalDataSource) {
340363

341364
@Override
342365
public CacheStats getCacheStats() {
343-
if (itemCache == null || allCache == null) {
366+
if (itemCache == null || allCache == null || cacheDisabled) {
344367
return null;
345368
}
346369
com.google.common.cache.CacheStats itemStats = itemCache.stats();
@@ -443,8 +466,9 @@ private boolean pollAvailabilityAfterOutage() {
443466
}
444467

445468
// Fall back to cache-based recovery if external store is not available/initialized
446-
// and we're in infinite cache mode
447-
if (cacheIndefinitely && allCache != null) {
469+
// and we're in infinite cache mode. Under FDv2 this branch is dead once
470+
// disableCache has run: the externalCache path above supersedes it.
471+
if (cacheIndefinitely && allCache != null && !cacheDisabled) {
448472
// If we're in infinite cache mode, then we can assume the cache has a full set of current
449473
// flag data (since presumably the data source has still been running) and we can just
450474
// write the contents of the cache to the underlying data store.

lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/WriteThroughStore.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,9 @@ private void maybeSwitchStore() {
149149
}
150150
synchronized (activeStoreLock) {
151151
activeReadStore = memoryStore;
152+
if (persistentStore instanceof DisableableCache) {
153+
((DisableableCache) persistentStore).disableCache();
154+
}
152155
}
153156
}
154157

lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/integrations/PersistentDataStoreBuilder.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,19 @@
3232
* </code></pre>
3333
*
3434
* In this example, {@code .url()} is an option specifically for the Redis integration, whereas
35-
* {@code cacheSeconds()} is an option that can be used for any persistent data store.
35+
* {@code cacheSeconds()} is an option that can be used for any persistent data store.
3636
* <p>
3737
* Note that this class is abstract; the actual implementation is created by calling
3838
* {@link Components#persistentDataStore(ComponentConfigurer)}.
39+
* <p>
40+
* Under the FDv2 data system, the cache options configured here ({@link #cacheTime(Duration)},
41+
* {@link #cacheSeconds(long)}, {@link #cacheMillis(long)}, {@link #cacheForever()},
42+
* {@link #noCaching()}, {@link #staleValuesPolicy(StaleValuesPolicy)},
43+
* {@link #recordCacheStats(boolean)}) only govern the brief bootstrap window before the in-memory
44+
* store has received its first full payload. Once the in-memory store takes over as the active
45+
* read source, the persistent-store cache is released and these settings have no further effect.
46+
* These options are kept for backward compatibility and may be deprecated in a future major
47+
* version.
3948
* @since 4.12.0
4049
*/
4150
public abstract class PersistentDataStoreBuilder implements ComponentConfigurer<DataStore> {

lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/PersistentDataStoreWrapperTest.java

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import static org.hamcrest.Matchers.greaterThan;
3636
import static org.hamcrest.Matchers.is;
3737
import static org.hamcrest.Matchers.nullValue;
38+
import static org.junit.Assert.assertNotNull;
3839
import static org.junit.Assert.assertNull;
3940
import static org.junit.Assert.fail;
4041
import static org.junit.Assume.assumeThat;
@@ -713,6 +714,115 @@ public void statusRemainsUnavailableIfStoreSaysItIsAvailableButInitFails() throw
713714
assertThat(core.initedCount.get(), greaterThan(initedCount));
714715
}
715716

717+
@Test
718+
public void disableCacheIsIdempotent() {
719+
assumeThat(testMode.isCached(), is(true));
720+
wrapper.disableCache();
721+
wrapper.disableCache(); // must not throw
722+
}
723+
724+
@Test
725+
public void disableCacheIsSafeOnUncachedWrapper() {
726+
assumeThat(testMode.isCached(), is(false));
727+
wrapper.disableCache(); // must not throw
728+
}
729+
730+
@Test
731+
public void getAfterDisableCacheReturnsCurrentCoreState() {
732+
assumeThat(testMode.isCached(), is(true));
733+
TestItem item1v1 = new TestItem("key", 1);
734+
TestItem item1v2 = new TestItem("key", 2);
735+
736+
core.forceSet(TEST_ITEMS, item1v1);
737+
// Prime the cache.
738+
assertThat(wrapper.get(TEST_ITEMS, item1v1.key), equalTo(item1v1.toItemDescriptor()));
739+
740+
wrapper.disableCache();
741+
742+
// Mutate the core behind the wrapper's back; if the cache were still
743+
// serving reads we would see the stale v1.
744+
core.forceSet(TEST_ITEMS, item1v2);
745+
assertThat(wrapper.get(TEST_ITEMS, item1v2.key), equalTo(item1v2.toItemDescriptor()));
746+
}
747+
748+
@Test
749+
public void getAllAfterDisableCacheReturnsCurrentCoreState() {
750+
assumeThat(testMode.isCached(), is(true));
751+
TestItem item1 = new TestItem("keyA", 1);
752+
TestItem item2 = new TestItem("keyB", 1);
753+
754+
core.forceSet(TEST_ITEMS, item1);
755+
// Prime the cache.
756+
Map<String, ItemDescriptor> primed = toItemsMap(wrapper.getAll(TEST_ITEMS));
757+
assertThat(primed.size(), is(1));
758+
759+
wrapper.disableCache();
760+
761+
core.forceSet(TEST_ITEMS, item2);
762+
Map<String, ItemDescriptor> afterDrop = toItemsMap(wrapper.getAll(TEST_ITEMS));
763+
assertThat(afterDrop.size(), is(2));
764+
}
765+
766+
@Test
767+
public void upsertAfterDisableCacheWritesThroughToCoreOnly() {
768+
assumeThat(testMode.isCached(), is(true));
769+
TestItem item = new TestItem("key", 1);
770+
771+
wrapper.disableCache();
772+
773+
assertThat(wrapper.upsert(TEST_ITEMS, item.key, item.toItemDescriptor()), is(true));
774+
// The write must have landed in the core.
775+
assertThat(core.data.get(TEST_ITEMS).get(item.key), equalTo(item.toSerializedItemDescriptor()));
776+
// And subsequent reads must reach the core (no repopulated cache).
777+
assertThat(wrapper.get(TEST_ITEMS, item.key), equalTo(item.toItemDescriptor()));
778+
}
779+
780+
@Test
781+
public void initAfterDisableCacheWritesThroughToCoreWithoutRepopulatingCache() {
782+
assumeThat(testMode.isCached(), is(true));
783+
TestItem itemA = new TestItem("keyA", 1);
784+
TestItem itemB = new TestItem("keyA", 2);
785+
786+
wrapper.disableCache();
787+
788+
wrapper.init(new DataBuilder().add(TEST_ITEMS, itemA).build());
789+
790+
assertThat(core.data.get(TEST_ITEMS).get(itemA.key), equalTo(itemA.toSerializedItemDescriptor()));
791+
792+
// Mutate the core behind the wrapper's back; if the cache had repopulated
793+
// we would still see itemA on the next read.
794+
core.forceSet(TEST_ITEMS, itemB);
795+
assertThat(wrapper.get(TEST_ITEMS, itemB.key), equalTo(itemB.toItemDescriptor()));
796+
}
797+
798+
@Test
799+
public void getCacheStatsAfterDisableCacheReturnsNull() {
800+
assumeThat(testMode.isCached(), is(true));
801+
// Build a wrapper with stats recording enabled so getCacheStats is non-null pre-disable.
802+
PersistentDataStoreWrapper w = new PersistentDataStoreWrapper(
803+
new MockPersistentDataStore(),
804+
testMode.getCacheTtl(),
805+
PersistentDataStoreBuilder.StaleValuesPolicy.EVICT,
806+
true,
807+
this::updateStatus,
808+
sharedExecutor,
809+
testLogger);
810+
try {
811+
assertNotNull(w.getCacheStats());
812+
w.disableCache();
813+
assertNull(w.getCacheStats());
814+
} finally {
815+
try { w.close(); } catch (IOException e) { /* ignore */ }
816+
}
817+
}
818+
819+
@Test
820+
public void closeAfterDisableCacheDoesNotThrow() throws IOException {
821+
assumeThat(testMode.isCached(), is(true));
822+
wrapper.disableCache();
823+
wrapper.close(); // safety belt; tearDown will also call close, which must be safe
824+
}
825+
716826
private void causeStoreError(MockPersistentDataStore core, PersistentDataStoreWrapper w) {
717827
core.unavailable = true;
718828
core.fakeError = new RuntimeException(FAKE_ERROR.getMessage());

0 commit comments

Comments
 (0)