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

Implement a caching mechanism to minimize the number of HTTP requests sent by the client #200

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

Conversation

agateblue
Copy link

In a project of mine, I end up doing a lot of requests to the public Musicbrainz API endpoint. Due to the throttle policies in place (or the heavy load that sometimes occur on this public instance), some of my requests end up raising a 503 error.

To partially adress that, I've implemented a basic cache mechanism, to store API results and return the API results from the cache when possible, effectively minimizing the number of HTTP requests made.

I'll let you have a look at the commits for more details, but the implementation is a follows:

  1. If a cache is configured, the _mb_request function will first try to retrieve the value from the cache
  2. If not present, the actual request is sent to the API
  3. When the API returns, the response is stored in the cache for further use

This implementation is agnostic regarding how the cache behave. A simple cache (DictCache) is provided to demonstrate the cache behaviour, but more complex caches (using real cache systems, such as Redis or Memcached) are totally doable.

The cache key for each response is built using the URL and arguments (such as includes), so there should not be any collision.

The build may fail on Travis, because the tests I wrote require the mock library. I've added it to the tox dependencies, but since it's not called on Travis, I'm not sure how to make the build install the library before running tests.

Documentation is attached in the commit, if you want more details.

Any feedback is welcome, there are probably a few things to fix before this can be merged :)

@alastair
Copy link
Owner

This is a great patch, thanks! I probably won't have time to review it until August, but I'm in favour of including something like this.

@agateblue
Copy link
Author

No worries, we're all pretty busy. Feel free to tell me if there is anything I can do to speed up / help with the reviewing process.

@mineo
Copy link
Collaborator

mineo commented Jul 1, 2016

Hi Eliot and others, if we happen to merge #199 before this, you can find a branch where I've already merged this on top of #199 and fixed the conflicts at https://github.com/mineo/python-musicbrainz-ngs/tree/request-reloaded-testmerge.

@agateblue
Copy link
Author

Hey @mineo, thank you for your hard work on this, I'm really happy to hear things are moving forward !

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