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

Cache programmatic API - support asynchronous value loader #30314

Merged
merged 1 commit into from
Jan 19, 2023
Merged

Cache programmatic API - support asynchronous value loader #30314

merged 1 commit into from
Jan 19, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jan 11, 2023

@mkouba mkouba requested a review from gwenneg January 11, 2023 16:01
@mkouba mkouba requested a review from gastaldi January 12, 2023 08:13
@mkouba mkouba marked this pull request as ready for review January 12, 2023 09:53
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba
Copy link
Contributor Author

mkouba commented Jan 13, 2023

@gwenneg I would really like to hear your opinion as well ;-)

@gwenneg
Copy link
Member

gwenneg commented Jan 13, 2023

Sorry, busy week 😄 I'll take a look shortly.

@mkouba
Copy link
Contributor Author

mkouba commented Jan 13, 2023

Sorry, busy week smile I'll take a look shortly.

No problem. Thanks a lot!

@mkouba
Copy link
Contributor Author

mkouba commented Jan 13, 2023 •
edited
Loading

@gwenneg I don't think that UnresolvedUniValue.INSTANCE is really needed. The CacheResultInterceptor can just use the Cache#getAsync() for async return types - see my last commit. But maybe I'm missing something obvious?

@mkouba mkouba requested a review from gwenneg January 13, 2023 13:44
@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot

This comment has been minimized.

Sign in to view
@gwenneg
Copy link
Member

gwenneg commented Jan 16, 2023

I need to take care of urgent matters, I'll review this as soon as I can (which may be tomorrow).

@mkouba
Copy link
Contributor Author

mkouba commented Jan 18, 2023

@gwenneg all your comments should be addressed now...

@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba mkouba requested a review from gwenneg January 18, 2023 13:28
@quarkus-bot

This comment has been minimized.

Sign in to view
gwenneg
gwenneg approved these changes Jan 18, 2023
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.

The new code in CaffeineCacheImpl contains one method reference and two lambdas. You may want to replace them with something else if they are still considered a bad practice in the Quarkus runtime code.

Otherwise, LGTM. Thanks!

@mkouba
Copy link
Contributor Author

mkouba commented Jan 18, 2023

The new code in CaffeineCacheImpl contains one method reference and two lambdas. You may want to replace them with something else if they are still considered a bad practice in the Quarkus runtime code.

Otherwise, LGTM. Thanks!

Extensive usage of lambdas is probably still considered a bad practice. Let me fix that.

@mkouba
Cache programmatic API - support asynchronous value loader
36b0c68 
- resolves quarkusio#30311
- CacheResultInterceptor - use Cache#getAsync() for async return types
- deprecate UnresolvedUniValue
@mkouba
Copy link
Contributor Author

mkouba commented Jan 18, 2023

The new code in CaffeineCacheImpl contains one method reference and two lambdas. You may want to replace them with something else if they are still considered a bad practice in the Quarkus runtime code.
Otherwise, LGTM. Thanks!

Extensive usage of lambdas is probably still considered a bad practice. Let me fix that.

Done.

@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 18, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 18, 2023

✔️ 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.

@mkouba mkouba merged commit 3a4b19d into quarkusio:main Jan 19, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 19, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Jan 19, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jan 19, 2023
This was referenced Mar 10, 2023
Closed
Merged
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

@gastaldi gastaldi gastaldi approved these changes

Assignees
No one assigned
Labels
area/cache kind/enhancement New feature or request
Projects
None yet
Milestone
3.0.0.Alpha4
Development

Successfully merging this pull request may close these issues.

Cache programmatic API - support asynchronous value loader
4 participants
@mkouba @gwenneg @gastaldi @ben-manes