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

Use CacheControl module for better caching #7

Merged
merged 15 commits into from Jan 25, 2019

Conversation

@b-m-f
Copy link
Contributor

b-m-f commented Jan 15, 2019

DONE

Implements 1 part of ubuntudesign/base-squad#119
Fixes #5, #6

QA

Here is a test script to make sure that the code works.
This needs a Redis server running at the standard config locally.

import time
import redis
from cachecontrol.caches.file_cache import FileCache
from cachecontrol.caches.redis_cache import RedisCache
from canonicalwebteam.http import CachedSession
from datetime import datetime


def create_redis_connection_pool(
    redis_port="6379", redis_host="localhost", redis_db=0
):
    return redis.ConnectionPool(host=redis_host, port=redis_port, db=redis_db)


pool = create_redis_connection_pool()
sess = CachedSession(redis_cache_pool=pool, fallback_cache_duration=10)
print(sess)
for i in range(9):
    print("start")
    print(datetime.now())
    response = sess.get("https://ehlers.berlin")

    print("is response from cache")
    print(response.from_cache)
    print(datetime.now())
    print(response.headers)
    time.sleep(10)
@Greggless

This comment has been minimized.

Copy link

Greggless commented Jan 16, 2019

-1

@b-m-f b-m-f force-pushed the b-m-f:master branch 3 times, most recently from 44231ca to 6f9d938 Jan 16, 2019

Show resolved Hide resolved setup.py Outdated
Show resolved Hide resolved setup.py Outdated
@tbille

This comment has been minimized.

Copy link

tbille commented Jan 16, 2019

There is a yolo file test 😄

Show resolved Hide resolved README.rst

@b-m-f b-m-f force-pushed the b-m-f:master branch from 6f9d938 to b03bdda Jan 16, 2019

@tbille

This comment has been minimized.

Copy link

tbille commented Jan 16, 2019

@b-m-f I am not sure if we want this in this PR but: we should also provide a way for the user to allow him not to use redis (in dev mode for ex) but keep the file cache system that we had.

@nottrobin any opinion on this?

@b-m-f b-m-f force-pushed the b-m-f:master branch from b03bdda to af4a5ef Jan 16, 2019

@tbille

This comment has been minimized.

Copy link

tbille commented Jan 16, 2019

@b-m-f

This comment has been minimized.

Copy link
Contributor Author

b-m-f commented Jan 16, 2019

@b-m-f I am not sure if we want this in this PR but: we should also provide a way for the user to allow him not to use redis (in dev mode for ex) but keep the file cache system that we had.

@nottrobin any opinion on this?

We could add two functions/classes for this. Standard cache and redis. Or pass an option to the function, that the dev could overwrite in the implementation?

@nottrobin

This comment has been minimized.

Copy link
Member

nottrobin commented Jan 16, 2019

@tbille: I am not sure if we want this in this PR but: we should also provide a way for the user to allow him not to use redis (in dev mode for ex) but keep the file cache system that we had.

I agree. I actually think the default should remain as a file-based cache, with the option to pass a redis cache object. Because obviously having a Redis cache backend is the harder thing to achieve, so the module should, by default, do the easy thing. This also allows us to merge and start using the new CacheControl features before we actually have a Redis set up.

@b-m-f: We could add two functions/classes for this. Standard cache and redis. Or pass an option to the function, that the dev could overwrite in the implementation?

I think the function you've created should remain, with simply an optional parameter for the RedisCache. If it's missing, then use the file-based cache.

I also think that the logic of setting up the connection (turning redis_port, redis_host, redis_db into a RedisCache) should be taken out of that function. Keeping this logic outside the function makes the function more single-purpose and improves the options for dependency injection.

So something like:

def create_cached_session(
    redis_cache=None,
    fallback_cache_duration=5,
):

We could then initially leave it up to the client to do:

redis_cache = RedisCache(
    connection_pool=redis.ConnectionPool(
        host=redis_host, port=redis_port, db=redis_db
    )
)
session = create_cached_session(redis_cache=redis_cache)

This doesn't feel too verbose to me, but if you feel that's too noisy you could create another helper function for creating that redis_cache.

@b-m-f b-m-f force-pushed the b-m-f:master branch from af4a5ef to be15cc1 Jan 16, 2019

@nottrobin

This comment has been minimized.

Copy link
Member

nottrobin commented Jan 16, 2019

@tbille: You can check this file https://github.com/canonical-websites/snapcraft.io/blob/20efef5cb66e2306d17330e9ddf84dc8c25b1de9/webapp/api/requests.py

What were you referring to here? Is there something in that file we should be porting over?

Show resolved Hide resolved tests/test_cache.py Outdated

@b-m-f b-m-f force-pushed the b-m-f:master branch from be15cc1 to 18e73b0 Jan 16, 2019

b-m-f added some commits Jan 23, 2019

@b-m-f b-m-f force-pushed the b-m-f:master branch 2 times, most recently from 414804f to 8e15a44 Jan 24, 2019

@b-m-f b-m-f force-pushed the b-m-f:master branch from 8e15a44 to e54a7b4 Jan 24, 2019

@b-m-f b-m-f force-pushed the b-m-f:master branch 2 times, most recently from 98ddecc to d4dac92 Jan 25, 2019

Show resolved Hide resolved .vscode/settings.json Outdated

@b-m-f b-m-f force-pushed the b-m-f:master branch from 3d36ac6 to e89fea0 Jan 25, 2019


def test_timeout_adapter(self):
session = CachedSession(
timeout=1.2, file_cache_directory=file_cache_directory

This comment has been minimized.

@nottrobin

nottrobin Jan 25, 2019

Member

In every other way, this now looks perfect. But unfortunately I'm still experiencing intermittent failures with this test.

I know it sucks, but could you just make this 2 seconds, and then use delay/3 and delay/1 for the test below? The values for me have to be this high to avoid intermittent failures.

If we still see issues down the line, we might just have to disable this test until we find a better way to do it.

This comment has been minimized.

@b-m-f

b-m-f Jan 25, 2019

Author Contributor

Have updated it and I created an issue at #9 .

Maybe I can label it with Help wanted?

@tbille

This comment has been minimized.

Copy link

tbille commented Jan 25, 2019

@b-m-f test file is back

@nottrobin
Copy link
Member

nottrobin left a comment

LGTM 👍 truly heroic effort. Thanks so much @b-m-f.

@b-m-f b-m-f merged commit 52aec5b into canonical-webteam:master Jan 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment