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

Redis cache "prefix" option erase the cache name / Prefix option is not a real prefix #44693

Closed
thibaultmeyer opened this issue Nov 25, 2024 · 10 comments · Fixed by #44856
Closed

Comments

@thibaultmeyer
Copy link
Contributor

thibaultmeyer commented Nov 25, 2024

Describe the bug

Hi,

The prefix option is not actually a prefix, but an overload of the cache name. This is either very strange or a problem with the name of the option.

Actually, when we define a prefix (for example, the application name, as several applications connect to a Redis cluster), we lose the cache name set in the annotation.

/cc @JackyAnn (#35094)

Expected behavior

By default, key in redis is cache:<cache-name>:<cache-key>. So, when the option quarkus.cache.redis.prefix is set to a value, by example ${quarkus.application.name}, we expect to have a cache key following this format : <custom-prefix>:<cache-name>:<cache-key> (or something like that).

Actual behavior

When the option quarkus.cache.redis.prefix is set to a value, by example ${quarkus.application.name}, we get a cache key following this format : <app-name>:<cache-key>.

(cache name set in the annotation by the developer is lost)

Output of uname -a or ver

Darwin plerome.local 23.6.0 Darwin Kernel Version 23.6.0: Mon Jul 29 21:16:46 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T8112 arm64

Output of java -version

penjdk version "22.0.2" 2024-07-16 OpenJDK Runtime Environment GraalVM CE 22.0.2+9.1 (build 22.0.2+9-jvmci-b01) OpenJDK 64-Bit Server VM GraalVM CE 22.0.2+9.1 (build 22.0.2+9-jvmci-b01, mixed mode, sharing)

Quarkus version or git rev

3.15.1

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.9.6 (bc0240f3c744dd6b6ec2920b3cd08dcc295161ae)

Additional information

Current implementation

String computeActualKey(String key) {

    if (cacheInfo.prefix != null) {
        return cacheInfo.prefix + ":" + key;
    } else {
        return "cache:" + getName() + ":" + key;
    }
}

Expected implementation (or something like that)

String computeActualKey(String key) {

    if (cacheInfo.prefix != null) {
        return cacheInfo.prefix + ":" + getName() + ":" + key;
        // or : return cacheInfo.prefix.replace("$cache-name", getName()) + ":" + key;
    } else {
        return "cache:" + getName() + ":" + key;
    }
}
@thibaultmeyer thibaultmeyer added the kind/bug Something isn't working label Nov 25, 2024
Copy link

quarkus-bot bot commented Nov 25, 2024

/cc @Ladicek (redis), @cescoffier (redis), @gwenneg (cache), @machi1990 (redis)

@Ladicek
Copy link
Contributor

Ladicek commented Nov 26, 2024

Not an expert here, but the current behavior seems intentional. It is even documented:

If not set, use "cache:$cache-name"

@thibaultmeyer
Copy link
Contributor Author

thibaultmeyer commented Nov 26, 2024

Not an expert here, but the current behavior seems intentional. It is even documented:

If not set, use "cache:$cache-name"

If we assume that the prefix is "cache:$cache-name", it should be possible to use "$cache-name" when i provide a custom prefix, but this it not working. "$cache-name" is not replaced by the value set in the annotation.

I think "$cache-name" (taken from the annotation) must no be part of the prefix and should always be present in the cache key. Only "cache" from "cache:$cache-name" should be the prefix.

@cescoffier
Copy link
Member

It works as intended.
The cache is caching value associated with a given cache-key. When stored in Redis, this key is prefixed to identify which Redis entries are managed by the cache. So, the actual Redis key is: $prefix:$cache-key. If the prefix is not set, it uses cache:$cache-name, which means that the final Redis key is: cache:$cache-name:$cache-key.

@thibaultmeyer
Copy link
Contributor Author

thibaultmeyer commented Nov 27, 2024

It works as intended.

Hi @cescoffier,

I'm sorry, i'm not agree, when "prefix" is set, we lost the cache-name set by the developers in the annotation. Is not what is attended when a prefix is set, it's clearly not a "prefix", but a "cache name" override (maybe 2 configuration keys are needed for two different purpose ?).

Value defined by developers in business code cannot be considered part of the prefix. Cache name set in the the annotation should alway be present (prefix used or not).

If really it works as intended, please add ability to use "$cache-name" in the custom prefix.

By example:

String computeActualKey(String key) {

    if (cacheInfo.prefix != null) {
        return cacheInfo.prefix.replace("$cache-name", getName()) + ":" + key;
    } else {
        return "cache:" + getName() + ":" + key;
    }
}

@cescoffier cescoffier removed the kind/bug Something isn't working label Nov 28, 2024
@cescoffier
Copy link
Member

It should be possible to introduce syntax when indicating the cache name placeholder. However, it cannot be $cache-name, as it will be interpreted as a config value placeholder. Maybe <cache-name>.

@radcortez do you know if we have something like this anywhere? (a placeholder in a config value that will be managed by the extension runtime code and note by the config system)

@radcortez
Copy link
Member

Maybe @Scheduled(cron = "{expr}")?

It ends up using Config, but manually. It could use anything to populate such expressions.

@cescoffier
Copy link
Member

Ah, yes, just {} without the $. Makes sense!

@thibaultmeyer
Copy link
Contributor Author

I've opened a merge request with a modification proposal (#44856).

@cescoffier
Copy link
Member

Thanks @thibaultmeyer! The PR looks good, I just asked for a simple test and a line in the doc in https://quarkus.io/guides/cache-redis-reference.

thibaultmeyer added a commit to thibaultmeyer/quarkus that referenced this issue Dec 1, 2024
thibaultmeyer added a commit to thibaultmeyer/quarkus that referenced this issue Dec 1, 2024
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 2, 2024
alex-pumpkin pushed a commit to alex-pumpkin/quarkus that referenced this issue Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants