Skip to content

Conversation

@sphill99
Copy link
Contributor

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