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

REFACTOR Caching #16

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

Conversation

hsssystems
Copy link

This PR fixes #15.

Details:

Instead of caching in the Reactive Stream layer we use the caching facilities of OkHttpClient. The behaviour differs from the former implementation in

  1. It caches :-) and does not double up page impressions
  2. It caches on disk, not in memory, so there are parameter for the caching directory and size
##Cache duration. When set to PT0M, cache is disabled. Min. Caching is one minute.
opencast.external-api.cache-expiration-duration=PT60M
opencast.external-api.cache-dir=/tmp/opencast-influxdb-adapter-cache
## Max. size in MB
opencast.external-api.cache-size=50
  1. Cache is persistent (restarting the application does not clear the cache). However clearing the cache is as easy as deleting the caching directory

@wsmirnow wsmirnow self-assigned this Nov 9, 2020
@wsmirnow wsmirnow self-requested a review November 9, 2020 16:38
@@ -20,8 +20,11 @@ adapter.invalid-publication-channels=internal
# opencast.external-api.uri=https://{organization}.api.opencast.com
# opencast.external-api.user=admin
# opencast.external-api.password=password
# opencast.external-api.max-cache-size=1000
# opencast.external-api.cache-expiration-duration=PT0M
##Cache duration. When set to PT0M, cache is disabled. Min. Caching is one minute.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the minimum cache time? Does PT0M disables the caching or not?

cacheDir = new File(opencastCacheDir);
}

long cacheSize = Long.valueOf(parsed.getProperty(OPENCAST_CACHE_SIZE,"10")) * 1024 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

The configuration set 50MB per default. We have to choose one, 10 or 50 but same default value.

Choose a reason for hiding this comment

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

I have matched this line to the default value.


private int maxAge;

public CacheInterceptor(int maxAge) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please ducument the argument what int does mean here.

Choose a reason for hiding this comment

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

I added a comment to explain the argument.

Response response = chain.proceed(chain.request());

CacheControl cacheControl = new CacheControl.Builder()
.maxAge(this.maxAge, TimeUnit.MINUTES)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does PT0M work with this? Or do we need an edge case with CacheControl.Builder#noStore()

Choose a reason for hiding this comment

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

Yes PT0M does work with this.

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.

Caching causes duplication of page impressions and does not cache
3 participants