-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: honor polling interval across process restarts #282
Changes from 3 commits
08dc14c
99c99a2
a0154a9
117a0cc
70faaa6
85a993a
1aacaf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -244,30 +244,44 @@ static final class PollingDataSourceBuilderImpl extends PollingDataSourceBuilder | |
implements DiagnosticDescription, DataSourceRequiresFeatureFetcher { | ||
@Override | ||
public DataSource build(ClientContext clientContext) { | ||
clientContext.getDataSourceUpdateSink().setStatus( | ||
clientContext.isInBackground() ? ConnectionInformation.ConnectionMode.BACKGROUND_POLLING : | ||
ClientContextImpl clientContextImpl = ClientContextImpl.get(clientContext); | ||
clientContextImpl.getDataSourceUpdateSink().setStatus( | ||
clientContextImpl.isInBackground() ? ConnectionInformation.ConnectionMode.BACKGROUND_POLLING : | ||
ConnectionInformation.ConnectionMode.POLLING, | ||
null | ||
); | ||
int actualPollIntervalMillis = clientContext.isInBackground() ? backgroundPollIntervalMillis : | ||
|
||
int pollInterval = clientContextImpl.isInBackground() ? backgroundPollIntervalMillis : | ||
pollIntervalMillis; | ||
int initialDelayMillis; | ||
|
||
long initialDelayMillis; | ||
if (clientContext.isInBackground() && Boolean.FALSE.equals(clientContext.getPreviouslyInBackground())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewers: This background/foregrounding distinction is no more! The only impact foreground/background has is on the polling interval. Now they are treated the same after that which results in some improvements in behavioral consistency. |
||
// If we're transitioning from foreground to background, then we don't want to do a | ||
// poll right away because we already have recent flag data. Start polling *after* | ||
// the first background poll interval. | ||
initialDelayMillis = backgroundPollIntervalMillis; | ||
} else { | ||
// If we're in the foreground-- or, if we're in the background but we started out | ||
// that way rather than transitioning-- then we should do the first poll right away. | ||
initialDelayMillis = 0; | ||
// get the last updated timestamp for this context | ||
PersistentDataStoreWrapper.PerEnvironmentData perEnvironmentData = clientContextImpl.getPerEnvironmentData(); | ||
String hashedContextId = LDUtil.urlSafeBase64HashedContextId(clientContextImpl.getEvaluationContext()); | ||
String fingerprint = LDUtil.urlSafeBase64Hash(clientContextImpl.getEvaluationContext()); | ||
Long lastUpdated = perEnvironmentData.getLastUpdated(hashedContextId, fingerprint); | ||
if (lastUpdated == null) { | ||
lastUpdated = 0L; // default to beginning of time | ||
} | ||
|
||
// To avoid unnecessarily frequent polling requests due to process or application lifecycle, we have added | ||
// this initial delay logic. Calculate how much time has passed since the last update, if that is less than | ||
// the polling interval, delay by the difference, otherwise 0 delay. | ||
long elapsedSinceUpdate = System.currentTimeMillis() - lastUpdated; | ||
initialDelayMillis = Math.max(pollInterval - elapsedSinceUpdate, 0); | ||
} | ||
ClientContextImpl clientContextImpl = ClientContextImpl.get(clientContext); | ||
|
||
return new PollingDataSource( | ||
clientContext.getEvaluationContext(), | ||
clientContext.getDataSourceUpdateSink(), | ||
clientContextImpl.getEvaluationContext(), | ||
clientContextImpl.getDataSourceUpdateSink(), | ||
initialDelayMillis, | ||
actualPollIntervalMillis, | ||
pollInterval, | ||
clientContextImpl.getFetcher(), | ||
clientContextImpl.getPlatformState(), | ||
clientContextImpl.getTaskExecutor(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,8 +39,6 @@ | |
* deferred listener calls are done via the {@link TaskExecutor} abstraction. | ||
*/ | ||
final class ContextDataManager { | ||
static final ContextHasher HASHER = new ContextHasher(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewers: ContextHasher was eliminated in favor of a static method. |
||
|
||
private final PersistentDataStoreWrapper.PerEnvironmentData environmentStore; | ||
private final int maxCachedContexts; | ||
private final TaskExecutor taskExecutor; | ||
|
@@ -57,14 +55,15 @@ final class ContextDataManager { | |
|
||
@NonNull private volatile LDContext currentContext; | ||
@NonNull private volatile EnvironmentData flags = new EnvironmentData(); | ||
@NonNull private volatile ContextIndex index = null; | ||
@NonNull private volatile ContextIndex index; | ||
|
||
ContextDataManager( | ||
@NonNull ClientContext clientContext, | ||
@NonNull PersistentDataStoreWrapper.PerEnvironmentData environmentStore, | ||
int maxCachedContexts | ||
) { | ||
this.environmentStore = environmentStore; | ||
this.index = environmentStore.getIndex(); | ||
this.maxCachedContexts = maxCachedContexts; | ||
this.taskExecutor = ClientContextImpl.get(clientContext).getTaskExecutor(); | ||
this.logger = clientContext.getBaseLogger(); | ||
|
@@ -121,7 +120,8 @@ private void initDataInternal( | |
boolean writeFlagsToPersistentStore | ||
) { | ||
List<String> removedContextIds = new ArrayList<>(); | ||
String contextId = hashedContextId(context); | ||
String contextId = LDUtil.urlSafeBase64HashedContextId(context); | ||
String fingerprint = LDUtil.urlSafeBase64Hash(context); | ||
EnvironmentData oldData; | ||
ContextIndex newIndex; | ||
|
||
|
@@ -133,9 +133,6 @@ private void initDataInternal( | |
|
||
oldData = flags; | ||
flags = newData; | ||
if (index == null) { | ||
index = environmentStore.getIndex(); | ||
} | ||
newIndex = index.updateTimestamp(contextId, System.currentTimeMillis()) | ||
.prune(maxCachedContexts, removedContextIds); | ||
index = newIndex; | ||
|
@@ -145,7 +142,7 @@ private void initDataInternal( | |
logger.debug("Removed flag data for context {} from persistent store", removedContextId); | ||
} | ||
if (writeFlagsToPersistentStore && maxCachedContexts != 0) { | ||
environmentStore.setContextData(contextId, newData); | ||
environmentStore.setContextData(contextId, fingerprint, newData); | ||
logger.debug("Updated flag data for context {} in persistent store", contextId); | ||
} | ||
environmentStore.setIndex(newIndex); | ||
|
@@ -241,8 +238,11 @@ public boolean upsert(@NonNull LDContext context, @NonNull Flag flag) { | |
updatedFlags = flags.withFlagUpdatedOrAdded(flag); | ||
flags = updatedFlags; | ||
|
||
String contextId = hashedContextId(context); | ||
environmentStore.setContextData(contextId, updatedFlags); | ||
String hashedContextId = LDUtil.urlSafeBase64HashedContextId(context); | ||
String fingerprint = LDUtil.urlSafeBase64Hash(context); | ||
environmentStore.setContextData(hashedContextId, fingerprint, updatedFlags); | ||
index = index.updateTimestamp(hashedContextId, System.currentTimeMillis()); | ||
environmentStore.setIndex(index); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewers: This code path is hit when a flag patch comes in. |
||
} | ||
|
||
Collection<String> updatedFlag = Collections.singletonList(flag.getKey()); | ||
|
@@ -292,10 +292,6 @@ public Collection<FeatureFlagChangeListener> getListenersByKey(String key) { | |
return res == null ? new HashSet<>() : res; | ||
} | ||
|
||
public static String hashedContextId(final LDContext context) { | ||
return HASHER.hash(context.getFullyQualifiedKey()); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For Reviewers: Moved to LDUtil |
||
/** | ||
* Attempts to retrieve data for the specified context, if any, from the persistent store. This | ||
* does not affect the current context/flags state. | ||
|
@@ -305,7 +301,7 @@ public static String hashedContextId(final LDContext context) { | |
*/ | ||
@VisibleForTesting | ||
public @Nullable EnvironmentData getStoredData(LDContext context) { | ||
return environmentStore.getContextData(hashedContextId(context)); | ||
return environmentStore.getContextData(LDUtil.urlSafeBase64HashedContextId(context)); | ||
} | ||
|
||
private void notifyFlagListeners(Collection<String> updatedFlagKeys) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewers: This class was a glorified static method, so I got rid of it and moved it to LDUtil. |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -138,7 +138,7 @@ private Request getDefaultRequest(LDContext ldContext) throws IOException { | |
// and methods like Uri.withAppendedPath, simply to minimize the amount of code that relies on | ||
// Android-specific APIs so our components are more easily unit-testable. | ||
URI uri = HttpHelpers.concatenateUriPath(pollUri, StandardEndpoints.POLLING_REQUEST_GET_BASE_PATH); | ||
uri = HttpHelpers.concatenateUriPath(uri, LDUtil.base64Url(ldContext)); | ||
uri = HttpHelpers.concatenateUriPath(uri, LDUtil.urlSafeBase64(ldContext)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For reviewers: Just a rename |
||
if (evaluationReasons) { | ||
uri = URI.create(uri.toString() + "?withReasons=true"); | ||
} | ||
|
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.
For reviewers: Updated from using
ClientContext
toClientContextImpl
type here to have access to an internal only object, namely thePerEnvironmentData
.