Conversation
…he initializer Introduce a CachedFlagStore interface in the subsystems package that provides read access to cached flag data by evaluation context. Add this as a nullable field to DataSourceBuildInputs and wire it through from FDv2DataSourceBuilder using PerEnvironmentData. This plumbing enables the upcoming FDv2 cache initializer to load persisted flags without depending on package-private types. Made-with: Cursor
…uilderImpl Add FDv2CacheInitializer that loads persisted flag data from the local cache as the first step in the initializer chain. Per CONNMODE 4.1.2, the result uses Selector.EMPTY and persist=false so the orchestrator continues to the polling initializer for a verified selector. Cache miss and no-store cases return interrupted status to move on without delay. Add CacheInitializerBuilderImpl in DataSystemComponents and comprehensive tests covering cache hit, miss, no store, exceptions, and shutdown behavior. Made-with: Cursor
Prepend the cache initializer to all connection modes per CONNMODE 4.1.1. Every mode now starts with a cache read before any network initializer, providing immediate flag values from local storage while the polling initializer fetches fresh data with a verified selector. Update initializer count assertions in DataSystemBuilderTest and FDv2DataSourceBuilderTest to reflect the new cache initializer. Made-with: Cursor
aaron-zeisler
force-pushed
the
aaronz/SDK-2070/cache-initializer
branch
from
363887c to
563ec22
Compare
| * allowing the cache initializer to load stored flags without depending on | ||
| * package-private types. | ||
| */ | ||
| public interface CachedFlagStore { |
There was a problem hiding this comment.
Is it acceptable for this interface to be public? It's public because it's been added to the DataSourceBuildInputs class, which is public. As part of the "Inputs" class, it gets passed down into DataSourceBuilder.build(), where it's used to build the cache initializer.
There was a problem hiding this comment.
PerEnvironmentData is the concrete class that implements this interface.
There was a problem hiding this comment.
From discussion:
Let's update this interface to align directly with PerEnvironmentData's getContextData() function. Let's move this interface into PersistentDataStoreWrapper.java (so it lives very close to PerEnvironmentData). Finally, does it make sense to just call this ReadOnlyPerEnvironmentData?
This means that the call sites will deal with EnvironmentData objects, so they'll need to be tweaked slightly.
There was a problem hiding this comment.
I've made this change in my latest push. The EnvironmentData class is private to the android package, but the "inputs" object is public in the subsystems package. So I decided to remove the cache from the "inputs" object and perform the dependency injection a different way.
| * A cache miss is reported as an {@link FDv2SourceResult.Status#interrupted} status, | ||
| * causing the orchestrator to move to the next initializer without delay. | ||
| */ | ||
| final class FDv2CacheInitializer implements Initializer { |
There was a problem hiding this comment.
Here's a summary of how the spec requirements and the js-core implementation were used when developing this class:
| Decision | Source | Reasoning |
|---|---|---|
Selector.EMPTY on cache result |
CONNMODE 4.1.2 | Cache is unverified; empty selector tells the orchestrator to continue to the polling initializer for a real selector |
persist=false on ChangeSet |
CONNMODE 4.1.2 | Don't re-write data we just read from cache |
Cache miss = interrupted |
js-core pattern | Fast failure so the orchestrator immediately moves on; interrupted is the correct signal (not terminalError, which would stop the chain) |
fdv1Fallback=false always |
Logic | Cache is local storage, no server headers are involved |
Nullable cachedFlagStore |
Testing pragmatism | Test contexts don't have a persistent store; graceful degradation avoids test setup burden |
In FDv2, the FDv2CacheInitializer handles cache loading as the first step in the initializer chain, making the cache load in ContextDataManager.switchToContext() redundant. Add a skipCacheLoad parameter to ContextDataManager and a setCurrentContext() method so that the FDv2 path sets the context without reading from cache, while the FDv1 path continues to load cached flags immediately. Made-with: Cursor
| } else { | ||
| // FDv1: load cached flags immediately while the data source fetches from the network. | ||
| contextDataManager.switchToContext(context); | ||
| } |
There was a problem hiding this comment.
We might be able to replace the if (config.dataSource instanceof FDv2DataSourceBuilder) { with something from ConnectivityManager here. I think ConnectivityManager has an internal variable that describes "FDv1 or FDv2?", but that internal variable isn't exposed via a getter function. Let me know if you think that's cleaner.
|
@tanderson-ld The 5th (and currently last) commit updates the code in LDClient and ContextDataManager. In FDv1, flags are loaded from the cache in ContextDataManager's constructor and also in LDClient's "identify" flow. But in FDv2, loading from cache in those places is redundant now that we have the cache initializer. I could defer this commit to a separate pull request if you feel that's cleaner. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
...rkly-android-client-sdk/src/main/java/com/launchdarkly/sdk/android/DataSystemComponents.java
Show resolved
Hide resolved
…entData Made-with: Cursor
…terrupted Cache miss and missing persistent store now return a "transfer of none" changeset (ChangeSetType.None with Selector.EMPTY) instead of an interrupted status. This fixes an OFFLINE mode regression where a cache miss left the SDK in a failed initialization state because no synchronizers follow to recover. Made-with: Cursor
| * Per CONNMODE 4.1.2, the cache initializer returns data with {@code persist=false} | ||
| * and {@link Selector#EMPTY} (no selector), so the orchestrator continues to the next | ||
| * initializer (polling) to obtain a verified selector from the server. This provides | ||
| * immediate flag values from cache while the network initializer fetches fresh data. |
There was a problem hiding this comment.
Remove details about orchestration and other initializers, this class should not know they exist for separation of concerns.
* Per CONNMODE 4.1.2, the cache initializer returns data with {@code persist=false}
* and {@link Selector#EMPTY} (no selector).
| * signals "I checked the source and there is nothing new" rather than an error, so the | ||
| * orchestrator records {@code anyDataReceived = true} and continues normally. This is | ||
| * critical for OFFLINE mode where no synchronizers follow: without it, a cache miss | ||
| * would leave the SDK in a failed initialization state. |
There was a problem hiding this comment.
This comment block has too much detail about other code that is not within this class's responsibility. Revise to only talk about why it says transfer of none in that case. Even 304 Not Modified is an HTTP status code and shouldn't be mentioned.
| * | ||
| * @param context the to switch to | ||
| */ | ||
| public void switchToContext(@NonNull LDContext context) { |
There was a problem hiding this comment.
Consider just passing the skip cache load bool into this method. Then you don't need to define the difference between "switch" and "set. "setCurrentContext" wouldn't be necessary.
| resultFuture.set(FDv2SourceResult.status( | ||
| FDv2SourceResult.Status.interrupted(e), false)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Something that might be worth testing.
Dispatching the execution onto the executor does have some overhead / delay associated with it as the other thread is scheduled to pick up the work. LDFutures.anyOf also has dispatching internally. If that overhead / delay is significant compared to the average time to read flags from the persistence, then this initializer may be better to just do the work right on the same thread as run() is called. This would undermine the "close-ability" of this initializer, but if the interaction with persistence is fast enough, do you really need "close-ability?
We may need this code path to be as fast as possible because a client initing with a timeout of 0 expects to get cached values as quickly as possible. We don't want to slow this use case down.
Unfortunately testing this on an emulator is not going to give good results so this would have to be done on a physical device.
This same concern does not apply to the polling or streaming initializers / synchronizers because the execution overhead is insignificant compared to the network latency (at least that is what we have been assuming).
There was a problem hiding this comment.
Thinking more, we may need to do more extensive testing around this "hot path to cached flags" because the FDv2 data source is already using a fixed thread pool executor which also has some overhead to start the thread that will call run().
There was a problem hiding this comment.
From my testing on my device, it looks like there is ~300us of delay. From other benchmarking of the Android SDKs startup performance on my phone, the total time to get from init starting to fdv2Source_start_start is ~48ms, so this task delay is 0.5% of startup time.
So not a big impact from a speed standpoint.
Conclusion: try to run on thread that called run() if trivial to keep thread count down, but if there is any concern it is an issue, then don't change.
|
We need to meet this requirement: For SDKs that are inherently waiting for cache today even with a timeout of 0 seconds, we need to ensure that in FDv2 that same SDK major version continues to wait for the cache initializer. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e1617fe. Configure here.
| logger.warn("Cache initializer failed: {}", e.toString()); | ||
| resultFuture.set(FDv2SourceResult.status( | ||
| FDv2SourceResult.Status.interrupted(e), false)); | ||
| } |
There was a problem hiding this comment.
Exception path inconsistent with cache miss in OFFLINE mode
Medium Severity
The PR description states "Exceptions during cache read are caught and treated as a cache miss," but the exception handler returns an interrupted status instead of a ChangeSetType.None changeset like the actual cache miss path does. In FDv2DataSource.runInitializers, a None changeset sets anyDataReceived = true, while an interrupted status does not. In OFFLINE mode (where the cache initializer is the only initializer and no synchronizers follow), a cache read exception causes initialization failure, whereas a cache miss succeeds — contradicting the stated design that both cases behave equivalently.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e1617fe. Configure here.
2 participants
Goal
Implement the FDv2 cache initializer so that cached flag data is loaded from local storage as the first step in every connection mode's initializer chain, providing immediate flag values while the network initializer fetches fresh data.
Approach
The FDv1 production code already loads cached flags during
ContextDataManager.switchToContext(). In FDv2, this cache loading is modeled as a dedicatedInitializer— the first in the chain — per CONNMODE spec 4.1.1. The implementation follows the same patterns asFDv2PollingInitializer(executor-based async, shutdown future race viaLDFutures.anyOf).A
CachedFlagStoreinterface bridges thesubsystemspackage with the package-privatePerEnvironmentData, similar to howSelectorSourcebridgesTransactionalDataStore. This is wired throughDataSourceBuildInputsand adapted inFDv2DataSourceBuilder.makeInputs().Key Behaviors
CHANGE_SETwithSelector.EMPTYandpersist=false, so the orchestrator applies the data immediately but continues to the polling initializer for a verified selector from the serverinterruptedstatus, causing the orchestrator to move to the next initializer without delayfdv1Fallbackis alwaysfalsesince the cache is local (no server headers involved)interruptedNot In Scope
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.
Note
Medium Risk
Changes FDv2 startup/identify behavior by inserting a new cache-loading initializer into every connection mode and altering how context switching interacts with persisted flag state. Risk is moderate because it affects initialization ordering and offline/early-flag availability across modes, though covered by new unit tests.
Overview
Adds an FDv2 cache initializer (
FDv2CacheInitializer) that reads persisted flag data per-context and returns it as an FDv2 changeset (withSelector.EMPTYandpersist=false) to provide immediate cached values while subsequent network initializers run.Wires this initializer into the default FDv2 mode table for all connection modes and updates
FDv2DataSourceBuilderto inject the persistent-store dependency at build time via a newPersistentDataStoreWrapper.ReadOnlyPerEnvironmentDataview.Adjusts
LDClient/ContextDataManagerto avoid redundant cache loading in FDv2 paths (newsetCurrentContext/skipCacheLoadflag) while keeping FDv1 behavior unchanged, and updates/extends tests to validate cache hit/miss/error/shutdown behavior and new initializer counts in mode tables.Reviewed by Cursor Bugbot for commit e1617fe. Bugbot is set up for automated code reviews on this repo. Configure here.