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

Added config to control the behavior of check on null values in redis-cache extension #42189

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anadinema
Copy link

Implementation to achieve #42184

Please let me know if this is not at all suitable, or if suitable then do let me know if any fixes needed or if I missed something.

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 28, 2024

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@anadinema anadinema changed the title added config to control the behavior of check on null values Added config to control the behavior of check on null values Jul 28, 2024
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@anadinema anadinema changed the title Added config to control the behavior of check on null values Added config to control the behavior of check on null values in redis-cache extension Jul 29, 2024
@anadinema anadinema force-pushed the feature/redis-cache-ignore-null-config branch from 1093258 to 57dd347 Compare July 29, 2024 08:12
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@geoand geoand requested a review from cescoffier August 1, 2024 12:59
@anadinema anadinema force-pushed the feature/redis-cache-ignore-null-config branch 2 times, most recently from 57dd347 to 5a2d66a Compare August 5, 2024 18:29
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Aug 6, 2024

@anadinema FYI, @cescoffier is on PTO so you'll get feedback when he's back.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link

github-actions bot commented Aug 12, 2024

🎊 PR Preview b7d6639 has been successfully built and deployed to https://quarkus-pr-main-42189-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

@cescoffier
Copy link
Member

LGTM.

Can you rebase your PR?

@anadinema anadinema force-pushed the feature/redis-cache-ignore-null-config branch from e85f7f1 to 5a2d66a Compare August 19, 2024 08:46
@anadinema anadinema force-pushed the feature/redis-cache-ignore-null-config branch from 5a2d66a to 9afd4a2 Compare August 19, 2024 08:46
@anadinema
Copy link
Author

anadinema commented Aug 19, 2024

@cescoffier done ✅ Hope it's good to merge.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@anadinema anadinema closed this Aug 19, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Aug 19, 2024
@anadinema anadinema reopened this Aug 19, 2024
@quarkus-bot quarkus-bot bot removed the triage/invalid This doesn't seem right label Aug 19, 2024
@cescoffier
Copy link
Member

I actually want to discuss this with @karesti and @Ladicek when they are back from PTO and handle null the same way in infinispan and redis cache.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 19, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9afd4a2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

You can consult the Develocity build scans.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 19, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 9afd4a2.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@karesti
Copy link
Member

karesti commented Aug 20, 2024

@cescoffier great that is happening! :) IMO this could be the cache extension SPI contract instead of having it specific to Redis since this looks more a generic behaviour of the caching api system rather than something specific to Redis

@cescoffier
Copy link
Member

@karesti yes.

Also, I would like to discuss a change the the cache SPI to distinguished a cached null value from a cache miss (as in term of metrics it's totally different)

@anadinema
Copy link
Author

@cescoffier let me know, in case the changes are to be made in the cache SPI itself. I can give it a try.

@Ladicek
Copy link
Contributor

Ladicek commented Sep 2, 2024

I'm not sure if Infinispan can store null values, but the only option in Redis would be to encode it using some sort of a sentinel value, one that is super unlikely to be used by applications. For example, a string "io.quarkus.cache.redis.nullValuePlaceholder". Otherwise, all null values will in fact be cache misses, which is bad.

@karesti
Copy link
Member

karesti commented Sep 2, 2024

I'm not sure if Infinispan can store null values, but the only option in Redis would be to encode it using some sort of a sentinel value, one that is super unlikely to be used by applications. For example, a string "io.quarkus.cache.redis.nullValuePlaceholder". Otherwise, all null values will in fact be cache misses, which is bad.

Infinispan can do it, we handle this case with protostream and with java too, to provide null value caching for spring cache

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

5 participants