Skip to content
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

New retry api for supporting timeouts before retrying again #363

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

sphill99
Copy link
Collaborator

This is code that implements a new api for a retry policy that supports a timeout before retrying. Firstly, I created a new interface that gets the amount of time to wait for that retry policy. To actually schedule it, I pass a ScheduledThreadPoolExecutor into the network, then schedule the request on that to be done after the appropriate amount of time.

Some things to note:

  • Currently I just initialize a ScheduledThreadPoolExecutor at declaration for the AsyncRequestQueue. We could add the ability for the user to customize this if we wanted, but wasn't sure if that was necessary.
  • There's a default TimeoutRetryPolicy that just takes in the timeout in the constructor and otherwise is the same as DefaultRetryPolicy.
  • Didn't add tests for this, since I didn't know how to set the ScheduledThreadPoolExecutor without using a real one.

@jpd236
Copy link
Collaborator

jpd236 commented Aug 24, 2020

Capturing some high-level thinking on this - we'll need to patch this in to make some further edits before merging:

  • We should just make the ScheduledThreadPoolExecutor required, I think - no reason to support the possibility that one isn't provided, since we can just have it be one thread. But we should be clearer that it should only run non-blocking tasks; if a blocking task needs to be scheduled, it should be posted to the blocking executor once the scheduled task is executed.
  • Can we share more of the ThreadFactory with those of the other executors?
  • We should make the new RetryPolicy an abstract class. Now that it's clear there are use cases for controlling other parameters between requests, it's certainly conceivable that we might want to add additional timeouts in the future. Given how painful it is to introduce a new timeout here, we should at least make it easier to add new ones if we decide they're necessary.
  • Given that, I think "TimeoutRetryPolicy" is probably too specific a name; would want something more generic (RequestRetryPolicy?) to cover other potential parameters we might add.

jpd236 added a commit to jpd236/volley that referenced this pull request Sep 26, 2020
This is a subset of google#363 which ensures a ScheduledExecutorService is available,
but does not yet define a new RetryPolicy interface. This gives us the
flexibility to introduce timer-based operations later without needing to change
the ExecutorFactory interface (which is now an abstract class to allow for future
additions, should any others be necessary).
jpd236 added a commit to jpd236/volley that referenced this pull request Sep 26, 2020
This is a subset of google#363 which ensures a ScheduledExecutorService is available,
but does not yet define a new RetryPolicy interface. This gives us the
flexibility to introduce timer-based operations later without needing to change
the ExecutorFactory interface (which is now an abstract class to allow for future
additions, should any others be necessary).
@jpd236
Copy link
Collaborator

jpd236 commented Sep 26, 2020

#367 splits out the subset of this involving providing a ScheduledExecutorService and handles the first two points above in making ScheduledExecutorService required and sharing more of the ThreadFactory with the other executors.

The broader RetryPolicy changes are still a good idea, but we can defer them until the rest of the Async stack is fleshed out.

jpd236 added a commit to jpd236/volley that referenced this pull request Sep 28, 2020
This is a subset of google#363 which ensures a ScheduledExecutorService is available,
but does not yet define a new RetryPolicy interface. This gives us the
flexibility to introduce timer-based operations later without needing to change
the ExecutorFactory interface (which is now an abstract class to allow for future
additions, should any others be necessary).
jpd236 added a commit that referenced this pull request Sep 29, 2020
…#367)

This is a subset of #363 which ensures a ScheduledExecutorService is available,
but does not yet define a new RetryPolicy interface. This gives us the
flexibility to introduce timer-based operations later without needing to change
the ExecutorFactory interface (which is now an abstract class to allow for future
additions, should any others be necessary).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants