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

Improved OkHttpConnector caching behavior #542

Merged
merged 10 commits into from
Oct 1, 2019

Conversation

bitwiseman
Copy link
Member

@bitwiseman bitwiseman commented Sep 11, 2019

In the Jenkins project we've seen a number of less than ideal behaviors by the OKHttp cache.
Some instances it seems bad data gets in and then gets stuck in the cache. In other cases it appears the cache obey the max-age=60 returned by GitHub and thus doesn't even look for new data from some requests.

The cache behavior is fine for client-side uses such as Android where optimizing for network usage is important. Users of this library are are generally less concerned with network usage - they turn on caching to avoid using up their itHub rate-limit budget.

This change modifies the default OkHttpConnect behavior to make OkHttp always check the cache against the GitHub site. OkHttp still sends the response ETag in the request which lets GitHub return a 304 Not Modified if nothing has changed and does not count that request against the users rate-limit. OkHttp handles this response automatically and returns the cached data.

This means that by using a bit more network bandwidth, this change guarantees up-to-date data while keeping the same rate-limit savings.

This change has no effect outside the scenario where OkHttpConnector is used with caching.

This is basically the same behavior added to Jenkins here.

@bitwiseman
Copy link
Member Author

I found this while doing some exploration of the OkHttpConnector with WireMock Snapshotting. Still need tests to show how this change effects query behavior.

@KostyaSha
Copy link
Contributor

For mocking better just disable cache

@bitwiseman
Copy link
Member Author

@KostyaSha
Generally true, but the point here is to show the behavior of caching and how the proposed change to OkHttpConnector improves the behavior.

Tests aside from this one will not have caching enabled.

@bitwiseman
Copy link
Member Author

Turned off the tests for now. Caching is painful.

@bitwiseman
Copy link
Member Author

I've removed the tests from CI, but left them in place for running locally. The data provided with the tests is accurate, but behavior changes when running from WireMock files. 😱

@bitwiseman bitwiseman marked this pull request as ready for review September 12, 2019 21:10
@bitwiseman bitwiseman changed the title Fix OkHttpConnector caching Improved OkHttpConnector caching behavior Sep 12, 2019
@bitwiseman bitwiseman merged commit 9a91cc4 into hub4j:master Oct 1, 2019
@bitwiseman bitwiseman deleted the cache-fix branch October 1, 2019 20:11
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