-
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?
Changes from all commits
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 |
---|---|---|
|
@@ -22,6 +22,8 @@ final class PollingDataSource implements DataSource { | |
private final DataSourceUpdateSink dataSourceUpdateSink; | ||
final long initialDelayMillis; // visible for testing | ||
final long pollIntervalMillis; // visible for testing | ||
private final int maxNumberOfPolls; | ||
int numberOfPollsRemaining; // visible for testing | ||
private final FeatureFetcher fetcher; | ||
private final PlatformState platformState; | ||
private final TaskExecutor taskExecutor; | ||
|
@@ -36,6 +38,7 @@ final class PollingDataSource implements DataSource { | |
* source will report success immediately as it is now running even if data has not been | ||
* fetched. | ||
* @param pollIntervalMillis interval in millis between each polling request | ||
* @param maxNumberOfPolls the maximum number of polling attempts, unlimited if negative is provided | ||
* @param fetcher that will be used for each fetch | ||
* @param platformState used for making decisions based on platform state | ||
* @param taskExecutor that will be used to schedule the polling tasks | ||
|
@@ -46,6 +49,7 @@ final class PollingDataSource implements DataSource { | |
DataSourceUpdateSink dataSourceUpdateSink, | ||
long initialDelayMillis, | ||
long pollIntervalMillis, | ||
int maxNumberOfPolls, | ||
FeatureFetcher fetcher, | ||
PlatformState platformState, | ||
TaskExecutor taskExecutor, | ||
|
@@ -55,6 +59,8 @@ final class PollingDataSource implements DataSource { | |
this.dataSourceUpdateSink = dataSourceUpdateSink; | ||
this.initialDelayMillis = initialDelayMillis; | ||
this.pollIntervalMillis = pollIntervalMillis; | ||
this.maxNumberOfPolls = maxNumberOfPolls; | ||
this.numberOfPollsRemaining = maxNumberOfPolls; | ||
this.fetcher = fetcher; | ||
this.platformState = platformState; | ||
this.taskExecutor = taskExecutor; | ||
|
@@ -63,15 +69,16 @@ final class PollingDataSource implements DataSource { | |
|
||
@Override | ||
public void start(final Callback<Boolean> resultCallback) { | ||
|
||
if (initialDelayMillis > 0) { | ||
// if there is an initial delay, we will immediately report the successful start of the data source | ||
if (maxNumberOfPolls == 0) { | ||
// If there are no polls to be made, we will immediately report the successful start of the data source. This | ||
// may seem strange, but one can think of this data source as behaving like a no-op in this configuration. | ||
resultCallback.onSuccess(true); | ||
return; | ||
} | ||
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: The PR I opened previously to add "polling interval across restarts" used When 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. TODO: add fix documentation. |
||
|
||
Runnable pollRunnable = () -> poll(resultCallback); | ||
logger.debug("Scheduling polling task with interval of {}ms, starting after {}ms", | ||
pollIntervalMillis, initialDelayMillis); | ||
logger.debug("Scheduling polling task with interval of {}ms, starting after {}ms, with max number of polls of {}", | ||
pollIntervalMillis, initialDelayMillis, maxNumberOfPolls); | ||
ScheduledFuture<?> task = taskExecutor.startRepeatingTask(pollRunnable, | ||
initialDelayMillis, pollIntervalMillis); | ||
currentPollTask.set(task); | ||
|
@@ -87,7 +94,19 @@ public void stop(Callback<Void> completionCallback) { | |
} | ||
|
||
private void poll(Callback<Boolean> resultCallback) { | ||
ConnectivityManager.fetchAndSetData(fetcher, context, dataSourceUpdateSink, | ||
resultCallback, logger); | ||
// poll if there is no max (negative number) or there are polls remaining | ||
if (maxNumberOfPolls < 0 || numberOfPollsRemaining > 0) { | ||
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 commentThe 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. |
||
|
||
// terminate if we have a max number of polls and no polls remaining | ||
if (maxNumberOfPolls >= 0 && numberOfPollsRemaining <= 0) { | ||
ScheduledFuture<?> task = currentPollTask.getAndSet(null); | ||
if (task != null) { | ||
task.cancel(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 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?)