Repository navigation
fix: Make FDv1 fallback TTL-based and recover to FDv2 after the TTL #239
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.
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
Draft
beekld
wants to merge
2
commits into
main
Choose a base branch
from
devin/1792430315-fdv1-fallback-ttl
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
87 changes: 87 additions & 0 deletions
87
lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/FDv1FallbackDirective.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| package com.launchdarkly.sdk.server; | ||
|
|
||
| import com.google.gson.JsonElement; | ||
| import com.google.gson.JsonObject; | ||
| import com.launchdarkly.sdk.internal.fdv2.payloads.FDv2Event; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.concurrent.ThreadLocalRandom; | ||
|
|
||
| /** | ||
| * Reads the TTL of an FDv1 fallback directive, which tells the SDK how long to stop using FDv2 before attempting to | ||
| * recover to it. The directive is carried by the {@code X-LD-FD-Fallback} response header, with its TTL in the | ||
| * {@code X-LD-FD-Fallback-TTL} header, or by the {@code protocolFallbackTTL} field of a {@code goodbye} event. | ||
| */ | ||
| final class FDv1FallbackDirective { | ||
| /** | ||
| * The maximum TTL, which is also the TTL used when the service supplies no usable TTL. | ||
| */ | ||
| static final Duration DEFAULT_TTL = Duration.ofHours(1); | ||
|
|
||
| private static final String PROTOCOL_FALLBACK_TTL = "protocolFallbackTTL"; | ||
|
|
||
| private FDv1FallbackDirective() { | ||
| } | ||
|
|
||
| /** | ||
| * Returns a service-supplied TTL as given if it is in the range (0, {@link #DEFAULT_TTL}]. Any other value, | ||
| * including null, is replaced by {@link #DEFAULT_TTL} minus a jitter of up to half of it, so that a fallback is | ||
| * never indefinite and SDKs that fell back together do not all attempt to recover at once. | ||
| * | ||
| * @param ttl the TTL supplied by the service, or null if none was supplied | ||
| * @return the TTL to use | ||
| */ | ||
| static Duration ttlOrDefault(Duration ttl) { | ||
| if (ttl != null && !ttl.isNegative() && !ttl.isZero() && ttl.compareTo(DEFAULT_TTL) <= 0) { | ||
| return ttl; | ||
| } | ||
| long ttlSeconds = DEFAULT_TTL.getSeconds(); | ||
| long jitterSeconds = ThreadLocalRandom.current().nextLong(ttlSeconds / 2 + 1); | ||
| return Duration.ofSeconds(ttlSeconds - jitterSeconds); | ||
| } | ||
|
|
||
| /** | ||
| * Reads a directive from the {@code X-LD-FD-Fallback} and {@code X-LD-FD-Fallback-TTL} response headers. The TTL | ||
| * header is a whole number of seconds. | ||
| * | ||
| * @param fallbackHeader the value of the {@code X-LD-FD-Fallback} header, or null | ||
| * @param ttlHeader the value of the {@code X-LD-FD-Fallback-TTL} header, or null | ||
| * @return the TTL to use, or null if the headers do not request fallback | ||
| */ | ||
| static Duration fromHeaders(String fallbackHeader, String ttlHeader) { | ||
| if (fallbackHeader == null || !fallbackHeader.equalsIgnoreCase("true")) { | ||
| return null; | ||
| } | ||
| if (ttlHeader == null) { | ||
| return ttlOrDefault(null); | ||
| } | ||
| try { | ||
| return ttlOrDefault(Duration.ofSeconds(Long.parseLong(ttlHeader.trim()))); | ||
| } catch (NumberFormatException e) { | ||
| return ttlOrDefault(null); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Reads a directive from a {@code goodbye} event. The presence of {@code protocolFallbackTTL} is itself the | ||
| * directive; its value is the TTL in seconds. | ||
| * | ||
| * @param goodbye the goodbye event | ||
| * @return the TTL to use, or null if the event has no {@code protocolFallbackTTL} | ||
| */ | ||
| static Duration fromGoodbye(FDv2Event goodbye) { | ||
| JsonElement data = goodbye.getData(); | ||
| if (data == null || !data.isJsonObject()) { | ||
| return null; | ||
| } | ||
| JsonObject object = data.getAsJsonObject(); | ||
| if (!object.has(PROTOCOL_FALLBACK_TTL) || object.get(PROTOCOL_FALLBACK_TTL).isJsonNull()) { | ||
| return null; | ||
| } | ||
| JsonElement ttl = object.get(PROTOCOL_FALLBACK_TTL); | ||
| if (!ttl.isJsonPrimitive() || !ttl.getAsJsonPrimitive().isNumber()) { | ||
| return ttlOrDefault(null); | ||
| } | ||
| return ttlOrDefault(Duration.ofSeconds((long) ttl.getAsDouble())); | ||
| } | ||
| } |
192 changes: 126 additions & 66 deletions
192
lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/FDv2DataSource.java
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, fdv1 fallback was using just
factory.block()andfactory.unblock(). Why do we need this newsuspendedForFDv1Fallbacklist?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this PR, fallback was one-way, so a plain
block()was enough. NowendFDv1Fallback()has to undo it. ButBlockedalso 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 wereAvailablewhen fallback started.A simpler option, if you'd prefer it: add a third state,
Suspended, toSynchronizerFactoryWithState.fdv1Fallback()would moveAvailable→SuspendedandendFDv1Fallback()would moveSuspended→Available. That gets rid of the list and keeps everything in the existing state model. Happy to switch to it.