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.

Sign up for GitHub

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

Jump to bottom

Exposing getIfPresent in the CaffeineCache #26863

Merged
merged 8 commits into from Aug 16, 2022
Merged

Exposing getIfPresent in the CaffeineCache #26863

merged 8 commits into from Aug 16, 2022

Conversation

ivansenic
Copy link
Contributor

@ivansenic ivansenic commented Jul 21, 2022 •
edited

Fixes #26730.

Implemented the exposure of the getIfPresent from the CaffeineCache, as described in the #26730.

As this is my first PR, I tried to follow which test(s) should be updated by looking at references to the CaffeineCache#keySet() and the only test actually testing this method is ProgrammaticApiTest.. Not sure if this is enough, but as I was missing a test case for the exception thrown in the value loader, I added that..

Also tried to extend the docs..

I am available for any suggestion and proposal for improvement..

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 21, 2022

Thanks for your pull request!

The title of 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!)
  • title should not contain an issue number (use Fix #1234 in the description instead)

This message is automatically generated by a bot.

@ivansenic ivansenic changed the title fixes #26730: exposing getIfPresent in the CaffeineCache Exposing getIfPresent in the CaffeineCache Jul 21, 2022
@ivansenic ivansenic mentioned this pull request Jul 21, 2022
Closed
@gastaldi gastaldi requested a review from gwenneg July 21, 2022 16:57
@gwenneg gwenneg self-assigned this Jul 22, 2022
@rsvoboda
Copy link
Member

@gwenneg CI workflows were not executed, can you approve their run?

gwenneg
gwenneg reviewed Jul 22, 2022
View reviewed changes
Copy link
Member

@gwenneg gwenneg left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR @ivansenic!

extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java Outdated Show resolved Hide resolved
extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java Outdated Show resolved Hide resolved
@ebullient ebullient added this to Review pending in Quarkus Documentation Jul 25, 2022
@ivansenic
Copy link
Contributor Author

Updated the PR as requested, but please look into #26863 (comment)

@ivansenic ivansenic requested review from gwenneg and gsmet August 1, 2022 13:43
@gwenneg
Copy link
Member

gwenneg commented Aug 1, 2022

I'll review this shortly (hopefully today).

@ivansenic
Copy link
Contributor Author

Rebased on the latest main..

gwenneg
gwenneg reviewed Aug 3, 2022
View reviewed changes
Copy link
Member

@gwenneg gwenneg left a comment

Choose a reason for hiding this comment

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

Thanks for your patience! I'm quite busy this week, sorry for the delay 😃

extensions/cache/runtime/src/main/java/io/quarkus/cache/runtime/caffeine/CaffeineCacheImpl.java Outdated Show resolved Hide resolved
docs/src/main/asciidoc/cache.adoc Outdated Show resolved Hide resolved
docs/src/main/asciidoc/cache.adoc Outdated Show resolved Hide resolved
extensions/cache/runtime/src/main/java/io/quarkus/cache/CaffeineCache.java Outdated Show resolved Hide resolved
@ivansenic ivansenic requested a review from gwenneg August 4, 2022 10:14
@ivansenic
Copy link
Contributor Author

Do I need to rebase on the latest master?

@gwenneg
Copy link
Member

gwenneg commented Aug 12, 2022

Do I need to rebase on the latest master?

Only in case of conflicts. It's not needed right now.

@ivansenic
Copy link
Contributor Author

OK all tasks done, I guess this can be merged.

@ivansenic ivansenic requested a review from gwenneg August 15, 2022 14:50
Quarkus Documentation automation moved this from Review pending to Reviewer approved Aug 16, 2022
gwenneg
gwenneg approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member

@gwenneg gwenneg left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you again for your patience 😄

@quarkus-bot

This comment has been minimized.

Sign in to view
@gwenneg
Copy link
Member

gwenneg commented Aug 16, 2022

One of the old tests failed but it's unrelated to this PR because it does not change the failing test or affect any of the existing caching APIs. I'll relaunch the tests to check if the failure happens consistently.

@gwenneg gwenneg merged commit d7b6e98 into quarkusio:main Aug 16, 2022
Quarkus Documentation automation moved this from Reviewer approved to Done Aug 16, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Aug 16, 2022
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Aug 16, 2022
@ivansenic ivansenic deleted the ise/#26730 branch August 18, 2022 09:28
@ivansenic
Copy link
Contributor Author

LGTM

Thank you again for your patience smile

No problem. Let me know if I can help with some other stuff in the future..

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

@ben-manes ben-manes ben-manes left review comments

@gwenneg gwenneg gwenneg approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees

@gwenneg gwenneg

Labels
area/cache area/documentation kind/enhancement New feature or request
Projects
Quarkus Documentation
Done
Milestone
2.12.0.CR1
Development

Successfully merging this pull request may close these issues.

Expose getIfPresent in the CaffeineCache
5 participants
@ivansenic @rsvoboda @gwenneg @ben-manes @gsmet