feature - adding ability to set the amount of bytes cached #38
Conversation
sinemetu1
force-pushed
the
feature-max-bytes-cached
branch
from
6ae7f9e
to
cbeff72
Compare
sinemetu1
force-pushed
the
feature-max-bytes-cached
branch
from
cbeff72
to
cf78275
Compare
|
I'm going to go ahead and merge this, but in the future, I'd like to use a mixin for this sort of functionality as this could be valuable in other scenarios. Specifically, the default memory cache could use a simple way to set the max size. I already have plans to create a LRU mixin that allows a max size and this effectively seems similar. @sinemetu1 Let me know it the LRU mentioned above and in #36 if implemented as a mixin would be sufficient. I might extract the functionality before a release so as to avoid overloading the FileCache with too many concerns. In theory, a FileCache is relatively safe to use unbounded b/c disk space is cheap. Limiting the amount of space is something that is slightly more specialized, which I'd like to code as a mixin if possible. Thanks for the PR! |
feature - adding ability to set the amount of bytes cached
… been read before caching. I copied over the is_fp_closed from urllib3 and reversed the ordering of the try / except in order to give preference to HTTPResponses before real file pointers.
After a review with @dstufft the PR wasn't as robust as I'd hoped. The use of warnings (which is a good idea!) should really be done via logging so it can be easily turned on/off. Also, @dstufft mentioned sys.getsizeof is not entirely reliable across all systems. Most importantly though, the cache assumes it is the only instance. On a system restart a new instance's size goes to 0 even though there might be cached items.
|
After a code review with @dstufft I've rolled this back. The biggest reason is that the size of the cache will revert to 0 when the process restarts. This kind of defeats the purpose! Also, I think a better tactic would be inherit or make a mixin class based on the FileCache and use that directly in your app. By doing so you avoid generalizaton for different operating systems and you can integrate whatever notification system you need. Also, if resetting the cache to 0 on start doesn't cause a problem, then that is great! Sorry about reverting this change and thanks for submitting the PR. |
Hi @ionrock,
I'm working on a feature for http://github.com/mozilla/mozregression in which we could set the maximum bytes that we cache using cachecontrol. This PR adds this capability to the FileCache class with a
max_bytesparam. The default is 1Gb.The implementation in this PR is using the
sys.getsizeof(value)to take care of the byte tracking. Another approach might be to do something with os.path.getsize() like this against the top cache dir.Let me know what you think! :)