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

Add additional cache support for OkHttp #133

Closed
wants to merge 2 commits into from

Conversation

andreynovikov
Copy link

Add cache support for OkHttp in the same format as LwHttp.

@@ -115,4 +116,57 @@ public boolean requestCompleted(boolean success) {

return success;
}

class InputOutputStream extends InputStream {
InputStream mInputStream;
Copy link
Collaborator

@devemux86 devemux86 Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the internal InputStream here since we already extend it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's a kind of proxy. Do you wish to use super instead?

Copy link
Collaborator

@devemux86 devemux86 Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't use internally a different InputStream type than the extended one, we could simplify the implementation?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we could make the inner class static?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll test it later.

@devemux86
Copy link
Collaborator

devemux86 commented Aug 15, 2016

Do we need another implementation since OkHttp provides its own response cache and already exists OkHttpFactory(HttpResponseCache)?
See OkHttpEngineTest for sample example.

Reference opensciencemap#89, opensciencemap#100

@andreynovikov
Copy link
Author

Yes, that cache caches complete HTTP responses. I've implemented the same type of cache as in LwHttp when tile image blob is cached. This:

  1. Lets interchange engines.
  2. Makes cache useful without HTTP at all.

@devemux86 devemux86 changed the title Add cache support for OkHttp Add additional cache support for OkHttp Aug 15, 2016
@devemux86 devemux86 added this to the 0.6.0 milestone Aug 15, 2016
@stleusc
Copy link

stleusc commented Aug 16, 2016

why would we need OkHttp cache without HTTP? And does it really make sense to think about someone changing the engine while keeping the cache? NO!
We should stay more generic and use what is there.
Use OkHttp built in caching...

Lastly, there is WAY newer version of OkHttp out there and before making changes like that we should upgrade to that.

@devemux86
Copy link
Collaborator

there is WAY newer version of OkHttp out there and before making changes like that we should upgrade to that.

I opened #138 for that.

@andreynovikov
Copy link
Author

Will return to it later after #138

@devemux86 devemux86 removed this from the 0.6.0 milestone Aug 19, 2016
@devemux86
Copy link
Collaborator

devemux86 commented Oct 28, 2016

@andreynovikov we can close this?
Need to address first #138 and any work can be posted on new vtm-http module.

@devemux86 devemux86 closed this Oct 28, 2016
@andreynovikov andreynovikov deleted the okhttp-cache branch July 19, 2017 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants