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

Use tiered cache in place of Caffeine cache #775

Open
3 tasks
pditommaso opened this issue Dec 18, 2024 · 10 comments · May be fixed by #783
Open
3 tasks

Use tiered cache in place of Caffeine cache #775

pditommaso opened this issue Dec 18, 2024 · 10 comments · May be fixed by #783
Assignees
Labels

Comments

@pditommaso
Copy link
Collaborator

pditommaso commented Dec 18, 2024

Tiered cache provides twofolds benefits: 1) the cache content is shared across all replicas, and 2) the cache content is should not be reloaded in across restarts.

The goal of this issue is to replace the local Caffeine cache with a tiered cache for the following services:

Supporting those service require however extending the current tiered cache API adding the ability to use Java object a cache keys other than plain strings as it is now, see here

abstract class AbstractTieredCache implements TieredCache<String,V>

This could be achieved introducing an interface like the following one

interface TieredCacheKey {

  String stableHash()

}

The concrete key class should take care of proving an implementation for stableHash, likely using sipHash algorithm.

@munishchouhan munishchouhan self-assigned this Dec 18, 2024
@munishchouhan
Copy link
Member

munishchouhan commented Dec 18, 2024

@pditommaso does tieredcache also addresses #747?
because in AwsEcrService we are using cache.synchronous() to avoid concurreny issue

// get the token from the cache, if missing the it's automatically
// fetch using the AWS ECR client
// FIXME https://github.com/seqeralabs/wave/issues/747
return cache.synchronous().get(new AwsCreds(accessKey,secretKey,region,isPublic))
}

@pditommaso
Copy link
Collaborator Author

No, it uses always Ceffeine behind the scene. We need Java 24 to revert that change

@munishchouhan
Copy link
Member

Since now we need to use string as key, there is an issue with loader

String load(AwsCreds creds) throws Exception {
return creds.ecrPublic
? getLoginToken1(creds.accessKey, creds.secretKey, creds.region)
: getLoginToken0(creds.accessKey, creds.secretKey, creds.region)
}
}

here loader is using key (AwsCreds) to compute the value, changing creds to string will not let us compute it anymore.

@munishchouhan
Copy link
Member

we can store awscreds as it is and generate token, when fetchs from cache

@pditommaso
Copy link
Collaborator Author

AwsCreds should implement TieredCacheKey as I was suggesting, then should be the same

@munishchouhan
Copy link
Member

yes, that i did here:

String stableHash() {
final h = Hashing.sipHash24().newHasher()
for( Object it : [accessKey, secretKey, region, ecrPublic] ) {
if( it!=null )
h.putUnencodedChars(it.toString())
h.putUnencodedChars('/')
}
return h.hash()
}

but loader requires AwsCreds and not a string because it needs compute token from it

@pditommaso
Copy link
Collaborator Author

This needs to be changed to take TieredCacheKey as key instead of string

abstract class AbstractTieredCache<V extends MoshiExchange> implements TieredCache<String,V> {

ei.

abstract class AbstractTieredCache<K extends TieredCacheKey, V extends MoshiExchange> implements TieredCache<K,V> {

  .. 
}


make sense ?

@munishchouhan
Copy link
Member

ok understood, thanks

@munishchouhan
Copy link
Member

Another issue is Caffeine cache loader is not a Funtion<>, it doesn't have apply Method

@pditommaso
Copy link
Collaborator Author

It should not impact, the trick is to replace in the internal implementation the string key with TieredCacheKey.stableHash

@munishchouhan munishchouhan linked a pull request Dec 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants