-
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: polling data source now supports one shot configuration #285
base: main
Are you sure you want to change the base?
Conversation
* {@link LDClient#init(Application, LDConfig, LDContext, int)} even if the SDK has cached feature flags. | ||
* | ||
* @return true if the client is able to retrieve flag data from LaunchDarkly or offline, false if the client has been | ||
* unable to up to this point. |
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 this documentation with feedback from #284.
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.
If the one shot poll is prevented (maxNumPolls = 0), are we still initialized?
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.
(Reading further it would appear yes, so maybe we need some statement for that?)
ConnectivityManager.fetchAndSetData(fetcher, context, dataSourceUpdateSink, | ||
resultCallback, logger); | ||
numberOfPollsRemaining--; // decrementing even when we have unlimited polls has no consequence | ||
} |
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: Decrementing with unlimited polls will underflow after 20,428 years. Seemed worth the simplified implementation.
resultCallback.onSuccess(true); | ||
return; | ||
} |
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: The PR I opened previously to add "polling interval across restarts" used initialDelayMillis > 0
to short circuit initialized to true. I realized that was incorrect after examining isInitialized behavior. This bug existed only in 5.5.0 which was released two weeks ago.
When maxNumberOfPolls == 0
(such as in the rate limited one shot case), isInitialized should be true because the flags are "as up to date as they can be in the current configuration", which is how isInitialized behaves currently. For example: when offline mode is set, isInitialized returns 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.
TODO: add fix documentation.
Requirements
I have added test coverage for new or changed functionality
TODO
I have followed the repository's pull request submission guidelines
I have validated my changes against all supported platform versions
TODO
Describe the solution you've provided
Adds polling data source one shot mode. Updated data source to support providing the max number of polls to make.
Updated builder to calculate number of polls to make based on one shot configuration.