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

Move cache lock timeout logic to CacheResultInterceptor #11271

Merged

Conversation

gwenneg
Copy link
Member

@gwenneg gwenneg commented Aug 6, 2020

This PR lays some groundwork for #8631.

With the programmatic caching API in mind, I moved the cache lock timeout logic from the CaffeineCache implementation to CacheResultInterceptor since this is a feature that only makes sense while using the annotations caching API.

Please note that many things should change with #8631 if it ends up being merged, including the return types of all CaffeineCache methods which will all be async and rely on Mutiny. The purpose of this PR is only to move some misplaced code without changing anything to the cache extension behavior and performances.

@ben-manes: Could you please confirm this PR doesn't do any harm in terms of performances?

@ben-manes
Copy link

This looks fine to me. It's unfortunate that it has to be a blocking call, I suppose? If the return value was a future then it would be nicely asynchronous to avoid wasting threads, and you could use future.copy().orTimeout(...) for your timed get.

@gwenneg
Copy link
Member Author

gwenneg commented Aug 7, 2020

Thanks for reviewing this @ben-manes! I don't see how the blocking call could be avoided here unfortunately. Regarding future.copy().orTimeout(...), I don't think this can be used for now since Quarkus still supports Java 8 AFAIK and these CompletableFuture methods were introduced with Java 9.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Could you avoid adding too many lambdas in runtime code? Thanks!

} catch (Exception e) {
throw new RuntimeException(e);
}
}, executor);
Copy link
Member

Choose a reason for hiding this comment

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

General comment on this class: we try to avoid lambdas in runtime code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for reviewing this!

I already knew about lambdas being the enemy in runtime code but there are only functional interfaces instances in this class (BiFunction for lines 35 and 51 and Supplier for line 76) which were already present before the PR (with a few differences). Are those nasty lambdas which should be eliminated as well?

Copy link
Member

Choose a reason for hiding this comment

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

You can do it in a subsequent PR if you prefer but I would get rid of all the lambdas in this class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed all lambdas from the extension runtime code, it should be good now.

@gsmet gsmet merged commit b9979aa into quarkusio:master Aug 13, 2020
@gsmet gsmet added this to the 1.8.0 - master milestone Aug 13, 2020
@gwenneg gwenneg deleted the issue-8140-move-cache-lock-timeout-logic branch August 13, 2020 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants