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

Possible enhancement: Allow setting request priorities #1361

Closed
vinaysshenoy opened this issue Feb 4, 2015 · 11 comments
Closed

Possible enhancement: Allow setting request priorities #1361

vinaysshenoy opened this issue Feb 4, 2015 · 11 comments
Labels
enhancement Feature not a bug

Comments

@vinaysshenoy
Copy link

I've been using Volley with an OkHttp stack for a while now, but I've been wanting to switch to a pure OkHttp stack for a while now(streaming response bodies, interceptors, multipart requests, ease of integration are a few of the reasons).

One of the reasons I have held of on this change is because Volley has one feature that OkHttp doesn't have yet: support for prioritizing requests. Would such a feature be acceptable as a Pull Request if I implement it?

@swankjesse
Copy link
Member

This is a tricky feature to do generally. Is policy fixed or pluggable? Does the default policy starve low priority requests? When is priority enforced? Do we short circuit low priority calls that would benefit from the cache? Is the priority shared with the HTTP/2 server?

I'm reluctant to solve this partially, and solving it generally is a lot of work. I'd prefer to punt on this for now.

@vinaysshenoy
Copy link
Author

AH, got it. I didn't think of that.

Anyway, this is something I need and I feel it would be a good addition to
the library. I'll draft up a set of reqs for the feature(considering
whatever you've mentioned) and update this issue. If you're okay with them,
I can implement this and submit a PR whenever it's ready.

On Thu, Feb 5, 2015 at 2:31 AM, Jesse Wilson notifications@github.com
wrote:

This is a tricky feature to do generally. Is policy fixed or pluggable?
Does the default policy starve low priority requests? When is priority
enforced? Do we short circuit low priority calls that would benefit from
the cache? Is the priority shared with the HTTP/2 server?

I'm reluctant to solve this partially, and solving it generally is a lot
of work. I'd prefer to punt on this for now.


Reply to this email directly or view it on GitHub
#1361 (comment).

Vinay S Shenoy
Android App Developer at Flipkart.com
+91-9538122356 | vinaysshenoy@gmail.com
LinkedIn : http://in.linkedin.com/in/vinaysshenoy

@swankjesse
Copy link
Member

Cool. I think we'll probably hold on priorities until we also make the backend async for HTTP/2. When things are async we suddenly get better control over which order tasks are completed.

@swankjesse swankjesse added the enhancement Feature not a bug label Feb 9, 2015
@swankjesse swankjesse added this to the Icebox milestone Feb 9, 2015
@ansman
Copy link
Contributor

ansman commented Mar 15, 2016

@swankjesse Is there a timeline for when the backend will be async?

@swankjesse
Copy link
Member

No timeline.

@iAviatorJose
Copy link

iAviatorJose commented Sep 15, 2016

@swankjesse is this feature implemented ???

@swankjesse
Copy link
Member

Not implemented.

@alexanderkjeldaas
Copy link

I don't quite understand the worries cited above. Couldn't the priority simply be forwarded to the HTTP/2 server and be interpreted by the server?

How the server interprets priorities is given by the HTTP/2 spec (they can be ignored), and whether requests are starved etc. is out of scope for the library.

The tcp socket buffer size is the only thing the client needs to have control of.

@swankjesse
Copy link
Member

OkHttp has a queue of pending requests not yet transmitted to the server. A good priority mechanism should probably influence the queue.

@6bangs
Copy link

6bangs commented Aug 8, 2017

For our use case, we only wanted new requests to be prioritized over old requests, specifically with regard to the ready deque (last in, first out). The easiest way to accomplish that was to change the add call in the Dispatcher to a push (specifically this line).

This seems to be a more sensible default for many applications, if a real prioritization feature isn't on the roadmap for the foreseeable future.

@swankjesse swankjesse marked this as a duplicate of #1361 Feb 18, 2018
@swankjesse
Copy link
Member

Duplicate of #404

@swankjesse swankjesse marked this as a duplicate of #404 Feb 18, 2018
@swankjesse swankjesse removed this from the Icebox milestone Nov 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature not a bug
Projects
None yet
Development

No branches or pull requests

6 participants