Repository navigation
Conversation
Co-Authored-By: Bee Klimt <bklimt@launchdarkly.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
| import java.util.concurrent.ThreadLocalRandom; | ||
|
|
||
| /** | ||
| * This type is currently experimental and not subject to semantic versioning. |
There was a problem hiding this comment.
This makes me nervous. Why does this need to be public?
There was a problem hiding this comment.
There wasn't a good reason for it. FDv2SourceResult is public and in another package, so the directive type ended up public just so the result could carry it. In be4646a it's package-private now (com.launchdarkly.sdk.server.FDv1FallbackDirective, static helpers only).
The only public additions left are on the already-experimental FDv2SourceResult, and they follow the spec's fallbackToFDv1 + Duration? fallbackTTL shape:
getFdv1FallbackTtl()- 2-arg
interrupted/terminalError/changeSet/goodbyeoverloads that take a nullableDuration
The data source applies the (0, 1h] check and the jittered default via FDv1FallbackDirective.ttlOrDefault, so the existing boolean factories still work: true with no TTL means the jittered default.
| /** | ||
| * Reads FDv1 fallback directives from the signals that can carry them. | ||
| */ | ||
| final class FDv1FallbackDirectives { |
There was a problem hiding this comment.
It's confusing to have FDv1FallbackDirective and FDv1FallbackDirectives. It would be nice if we could merge them.
There was a problem hiding this comment.
Merged in be4646a. There's now a single package-private FDv1FallbackDirective with ttlOrDefault, fromHeaders, and fromGoodbye, and its tests are in FDv1FallbackDirectiveTest.
| factory.unblock(); | ||
| } else if (factory.getState() == SynchronizerFactoryWithState.State.Available) { | ||
| factory.block(); | ||
| suspendedForFDv1Fallback.add(factory); |
There was a problem hiding this comment.
Previously, fdv1 fallback was using just factory.block() and factory.unblock(). Why do we need this new suspendedForFDv1Fallback list?
There was a problem hiding this comment.
Before this PR, fallback was one-way, so a plain block() was enough. Now endFDv1Fallback() has to undo it. But Blocked also means "failed terminally" (TERMINAL_ERROR → blockCurrentSynchronizer(), "will not be used again until application restart"), so on recovery we can't just unblock every non-FDv1 synchronizer. That would bring back synchronizers that had failed permanently, for example after a 401. The list remembers which ones were Available when fallback started.
A simpler option, if you'd prefer it: add a third state, Suspended, to SynchronizerFactoryWithState. fdv1Fallback() would move Available → Suspended and endFDv1Fallback() would move Suspended → Available. That gets rid of the list and keeps everything in the existing state model. Happy to switch to it.
…its helpers Co-Authored-By: Bee Klimt <bklimt@launchdarkly.com>
Requirements
Related issues
SDK-2987 (part of SDK-2935). Spec: sdk-specs#251 (DATASYSTEM v2 1.6). Reference: cpp-sdks#631.
Describe the solution you've provided
Makes the server SDK's FDv1 fallback directive TTL-based. Fallback no longer lasts forever, and the SDK goes back to FDv2 when the TTL is up.
X-LD-FD-Fallback-TTL(polling and streaming) and fromgoodbye.protocolFallbackTTL. If a goodbye hasprotocolFallbackTTL, that counts as a directive by itself.FDv1FallbackDirective.Implementation details
FDv2DataSourcerun thread, and only results from the active synchronizer get applied. When no synchronizer replaces the active one,SourceManager.getNextAvailableSynchronizerAndSetActivenow closes it. Without this, the FDv2 synchronizer that sent the directive would stay connected while the SDK waits out the TTL with no FDv1 fallback.FDv1FallbackExpiryConditionis a new condition type. While fallback is engaged, it is the only condition on the FDv1 synchronizer, so the normal fallback/recovery conditions don't apply. It resets its timer whenever it sees a result that carries a directive.start()stays pending until FDv2 recovers instead of finishing withfalse, soLDClientstartup waits up tostartWait. If there are no FDv2 synchronizers to go back to, the FDv1 synchronizer just keeps running.FDv2SourceResult:getFdv1FallbackTtl()and 2-arginterrupted/terminalError/changeSet/goodbyeoverloads that take a nullableDuration(non-null means the result carries a directive with that TTL). The existingboolean fdv1Fallbackfactories still work;truewith no TTL means the jittered default.Describe alternatives you've considered
Parsing
protocolFallbackTTLinlib/shared/internal'sGoodbye/FDv2ActionGoodbye. This PR deliberately leavesGoodbye.javaunchanged. The server SDK depends on the releasedlaunchdarkly-java-sdk-internal(1.12.2), so a change there would need its own internal release and then a bump PR before the server could use it. Instead, the server readsprotocolFallbackTTLstraight from the goodbye event's JSON (FDv1FallbackDirective.fromGoodbye). That parsing can move intoGoodbyein a later change if reviewers want it.Additional context
No contract test changes, because the sdk-test-harness has no fallback-TTL coverage yet.
Link to Devin session: https://app.devin.ai/sessions/cdc21935a71a406bbe1b22dba462357c
Open in Devin Desktop: https://app.devin.ai/desktop/session/cdc21935a71a406bbe1b22dba462357c?variant=devin
Requested by: @beekld