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

Infinispan Cache Extension #39836

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Conversation

karesti
Copy link
Member

@karesti karesti commented Apr 2, 2024

  • Deprecates custom annotations
  • Implements the cache extension SPI

Closes #31896
Closes #29571

Depends on #40038

@karesti karesti requested a review from geoand April 2, 2024 14:29
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/infinispan Infinispan labels Apr 2, 2024
@quarkus-bot quarkus-bot bot added this to To do in Quarkus Documentation Apr 2, 2024
@karesti karesti requested a review from wburns April 2, 2024 14:35
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@karesti karesti force-pushed the infinispan-cache-extension branch from 4634de7 to ce98f06 Compare April 2, 2024 16:10
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

I don't know a ton about the Infinispan implementation, but the integration with Quarkus seems to do what we are doing in Redis, so it makes sense

@karesti karesti force-pushed the infinispan-cache-extension branch from ce98f06 to 1e336be Compare April 3, 2024 09:20
@quarkus-bot

This comment has been minimized.

@karesti karesti force-pushed the infinispan-cache-extension branch from 1e336be to 1af8976 Compare April 3, 2024 09:43
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@karesti karesti force-pushed the infinispan-cache-extension branch from 1af8976 to 9599407 Compare April 3, 2024 10:07
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@karesti karesti force-pushed the infinispan-cache-extension branch from 9599407 to 6d4e1bf Compare April 3, 2024 11:47
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@karesti
Copy link
Member Author

karesti commented Apr 3, 2024

@gsmet Guillaume could your eyes give some light to this failure ? i've run the /update-extension-dependencies.sh script locally but there aren't more apparent changes needed and build still fails. I can't see what I'm missing here

@gsmet gsmet force-pushed the infinispan-cache-extension branch from 6d4e1bf to b85c6d5 Compare April 3, 2024 13:22
@quarkus-bot

This comment has been minimized.

.github/native-tests.json Outdated Show resolved Hide resolved
Future<Uni<String>> thread1 = fork(() -> cache.get(id, key -> {
try {
barrier.await(10, TimeUnit.SECONDS);
barrier.await(10, TimeUnit.SECONDS);
Copy link
Member

Choose a reason for hiding this comment

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

why twice?

Copy link
Member Author

@karesti karesti Apr 17, 2024

Choose a reason for hiding this comment

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

this is a test coming from Spring Cache integration in the infinispan repo that tests the same logic. In order to avoid it to be a flaky test, as @jabolina explained to me, one is to first make sure we are inside the lambda, for line 189. The second to wait inside the lambda until we issue the second request on line 192

Copy link
Member

Choose a reason for hiding this comment

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

I would add that explanation in the code - because you can be sure I would ask the same question in 1 month... or one week, or even maybe tomorrow.


@Override
public Uni<Void> invalidate(Object key) {
return Uni.createFrom().completionStage(remoteCache.removeAsync(key));
Copy link
Member

Choose a reason for hiding this comment

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

That is not correct.
This invokes the "removeAsync" method immediately, and not at subscription time.

You need () -> removeCache.removeAsync(key)) (for all methods)

In addition, you may have to deal with the context, as your event is going to be emitted on a infinispan thread, which is not the caller context. At least add tests (the interceptor is likely going to do that already, but better verify).

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a new integration test, what else should I add to test about the context ?

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

Copy link

github-actions bot commented Apr 16, 2024

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@karesti
Copy link
Member Author

karesti commented Apr 17, 2024

@cescoffier I pushed changes

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.withEmptyApplication()
.withConfigurationResource("empty-application-infinispan-client.properties");
Copy link
Member

Choose a reason for hiding this comment

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

We may have a better way to create empty assets, but I can't remember the API.

Future<Uni<String>> thread1 = fork(() -> cache.get(id, key -> {
try {
barrier.await(10, TimeUnit.SECONDS);
barrier.await(10, TimeUnit.SECONDS);
Copy link
Member

Choose a reason for hiding this comment

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

I would add that explanation in the code - because you can be sure I would ask the same question in 1 month... or one week, or even maybe tomorrow.

Quarkus Documentation automation moved this from Review pending to Reviewer approved Apr 21, 2024
@karesti
Copy link
Member Author

karesti commented Apr 22, 2024

@cescoffier I'm going to add the comment in the test and push.
Is there anything else preventing the merge at this point ?

* Deprecates custom annotations
* Implements the cache extension SPI
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 22, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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.

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

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 22, 2024

Status for workflow Quarkus CI

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

✅ 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.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/websockets-next/server/deployment

io.quarkus.websockets.next.test.openconnections.OpenConnectionsTest.testOpenConnections - History

  • Condition with Lambda expression in io.quarkus.websockets.next.test.utils.WSClient was not fulfilled within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.websockets.next.test.utils.WSClient was not fulfilled within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.websockets.next.test.utils.WSClient.waitForMessages(WSClient.java:102)
	at io.quarkus.websockets.next.test.openconnections.OpenConnectionsTest.testOpenConnections(OpenConnectionsTest.java:69)

⚙️ JVM Tests - JDK 21

📦 extensions/websockets-next/server/deployment

io.quarkus.websockets.next.test.openconnections.OpenConnectionsTest.testOpenConnections - History

  • Condition with Lambda expression in io.quarkus.websockets.next.test.utils.WSClient was not fulfilled within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.websockets.next.test.utils.WSClient was not fulfilled within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.websockets.next.test.utils.WSClient.waitForMessages(WSClient.java:102)
	at io.quarkus.websockets.next.test.openconnections.OpenConnectionsTest.testOpenConnections(OpenConnectionsTest.java:69)

@karesti
Copy link
Member Author

karesti commented Apr 23, 2024

@geoand hey, do you spot anything missing at this point preventing the merge ?

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM

@geoand geoand merged commit a3d1568 into quarkusio:main Apr 23, 2024
54 checks passed
Quarkus Documentation automation moved this from Reviewer approved to Done Apr 23, 2024
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 23, 2024
@quarkus-bot quarkus-bot bot added kind/bugfix kind/enhancement New feature or request labels Apr 23, 2024
@karesti karesti deleted the infinispan-cache-extension branch April 23, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/infinispan Infinispan area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure kind/bugfix kind/enhancement New feature or request release/noteworthy-feature triage/flaky-test
4 participants