-
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 all 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
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ public class LDClientEndToEndTest { | |
private Application application; | ||
private MockWebServer mockPollingServer; | ||
private URI mockPollingServerUri; | ||
private final PersistentDataStore store = new InMemoryPersistentDataStore(); | ||
private PersistentDataStore store; | ||
|
||
@Rule | ||
public final ActivityScenarioRule<TestActivity> testScenario = | ||
|
@@ -65,6 +65,11 @@ public void setUp() { | |
}); | ||
} | ||
|
||
@Before | ||
public void before() { | ||
store = new InMemoryPersistentDataStore(); | ||
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: Tests were affecting each other's stores. Now they don't. |
||
} | ||
|
||
@After | ||
public void after() throws IOException { | ||
mockPollingServer.close(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -244,30 +244,36 @@ static final class PollingDataSourceBuilderImpl extends PollingDataSourceBuilder | |
implements DiagnosticDescription, DataSourceRequiresFeatureFetcher { | ||
@Override | ||
public DataSource build(ClientContext clientContext) { | ||
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: Updated from using |
||
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; | ||
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 | ||
} | ||
ClientContextImpl clientContextImpl = ClientContextImpl.get(clientContext); | ||
|
||
// 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; | ||
long initialDelayMillis = Math.max(pollInterval - elapsedSinceUpdate, 0); | ||
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: These code changes are the meat and potatoes of the PR. |
||
|
||
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(); | ||
|
@@ -120,8 +119,9 @@ private void initDataInternal( | |
@NonNull EnvironmentData newData, | ||
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,26 +133,27 @@ private void initDataInternal( | |
|
||
oldData = flags; | ||
flags = newData; | ||
if (index == null) { | ||
index = environmentStore.getIndex(); | ||
} | ||
newIndex = index.updateTimestamp(contextId, System.currentTimeMillis()) | ||
.prune(maxCachedContexts, removedContextIds); | ||
index = newIndex; | ||
|
||
for (String removedContextId: removedContextIds) { | ||
environmentStore.removeContextData(removedContextId); | ||
logger.debug("Removed flag data for context {} from persistent store", removedContextId); | ||
} | ||
if (writeFlagsToPersistentStore && maxCachedContexts != 0) { | ||
environmentStore.setContextData(contextId, newData); | ||
if (writeFlagsToPersistentStore) { | ||
List<String> removedContextIds = new ArrayList<>(); | ||
newIndex = index.updateTimestamp(contextId, System.currentTimeMillis()) | ||
.prune(maxCachedContexts, removedContextIds); | ||
index = newIndex; | ||
|
||
for (String removedContextId: removedContextIds) { | ||
environmentStore.removeContextData(removedContextId); | ||
logger.debug("Removed flag data for context {} from persistent store", removedContextId); | ||
} | ||
|
||
environmentStore.setContextData(contextId, fingerprint, newData); | ||
environmentStore.setIndex(newIndex); | ||
|
||
if (logger.isEnabled(LDLogLevel.DEBUG)) { | ||
logger.debug("Stored context index is now: {}", newIndex.toJson()); | ||
} | ||
|
||
logger.debug("Updated flag data for context {} in persistent store", contextId); | ||
} | ||
environmentStore.setIndex(newIndex); | ||
} | ||
|
||
if (logger.isEnabled(LDLogLevel.DEBUG)) { | ||
logger.debug("Stored context index is now: {}", newIndex.toJson()); | ||
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: Android was updating the index timestamp when switching context. This is not consistent with JS, Flutter, and the iOS SDK. Updated this logic so that the index is only updated when new flag data needs to be written to persistence. |
||
} | ||
|
||
// Determine which flags were updated and notify listeners, if any | ||
|
@@ -241,8 +242,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 +296,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 +305,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) { | ||
|
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: Associated class was removed.