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

Cache the item emitted by a Uni instead of the Uni itself #16608

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

gwenneg
Copy link
Member

@gwenneg gwenneg commented Apr 18, 2021

/**
* Tests the {@link CacheResult} annotation on methods returning {@link Uni}.
*/
public class UniValueTest {
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the log output for this test:

[INFO] Running io.quarkus.cache.test.runtime.UniValueTest
2021-04-18 02:17:52,879 INFO  [io.quarkus] (main) Quarkus 999-SNAPSHOT on JVM started in 0.486s. Listening on: http://localhost:8081
2021-04-18 02:17:52,879 INFO  [io.quarkus] (main) Profile test activated.
2021-04-18 02:17:52,880 INFO  [io.quarkus] (main) Installed features: [cache, cdi, mutiny, smallrye-context-propagation]
2021-04-18 02:17:53,156 DEBUG [io.qua.cac.run.CacheResultInterceptor] (main) Loading entry with key [key] from cache [test-cache]
2021-04-18 02:17:53,160 DEBUG [io.qua.cac.run.CacheResultInterceptor] (main) Adding UncomputedUniValue entry with key [key] into cache [test-cache]
2021-04-18 02:17:53,166 DEBUG [io.qua.cac.run.CacheResultInterceptor] (main) Loading entry with key [key] from cache [test-cache]
2021-04-18 02:17:53,169 DEBUG [io.qua.cac.run.caf.CaffeineCache] (main) Replacing Uni value entry with key [key] into cache [test-cache]
2021-04-18 02:17:53,169 DEBUG [io.qua.cac.run.caf.CaffeineCache] (main) Replacing Uni value entry with key [key] into cache [test-cache]
2021-04-18 02:17:53,170 DEBUG [io.qua.cac.run.CacheResultInterceptor] (main) Loading entry with key [key] from cache [test-cache]
2021-04-18 02:17:53,170 DEBUG [io.qua.cac.run.CacheResultInterceptor] (main) Loading entry with key [another-key] from cache [test-cache]
2021-04-18 02:17:53,170 DEBUG [io.qua.cac.run.CacheResultInterceptor] (main) Adding UncomputedUniValue entry with key [another-key] into cache [test-cache]
2021-04-18 02:17:53,170 DEBUG [io.qua.cac.run.caf.CaffeineCache] (main) Replacing Uni value entry with key [another-key] into cache [test-cache]
2021-04-18 02:17:53,215 INFO  [io.quarkus] (main) Quarkus stopped in 0.044s
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.723 s - in io.quarkus.cache.test.runtime.UniValueTest

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 18, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 311d99d

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs
🚫 Devtools Tests - JDK ${{ matrix.java.name }}
🚫 Devtools Tests - JDK 11 Windows
🚫 Gradle Tests - JDK 11 ${{ matrix.os.family }}
🚫 JVM Tests - JDK ${{ matrix.java.name }}
🚫 JVM Tests - JDK 11 Windows
🚫 Maven Tests - JDK ${{ matrix.java.name }}
🚫 Maven Tests - JDK 11 Windows
🚫 MicroProfile TCKs Tests
🚫 Native Tests - ${{ matrix.category }}
🚫 Native Tests - Windows - ${{ matrix.category }}

@gwenneg gwenneg force-pushed the issue-16607-cache-emitted-uni-item branch from 311d99d to 2a577df Compare April 18, 2021 00:37
@CacheResult(cacheName = "test-cache")
public Uni<String> cachedMethod(String key) {
invocations++;
return Uni.createFrom().item(() -> new String());
Copy link
Member

Choose a reason for hiding this comment

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

Can you also check that the number of subscriptions is equal to the number of invocations?

return Uni.createFrom().item(() -> {
   subscriptions++;
   return "" + subscriptions;
}

Copy link
Member Author

@gwenneg gwenneg Apr 18, 2021

Choose a reason for hiding this comment

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

Thanks for the review @cescoffier.

I added the subcriptions check. The number of subscriptions is not always equal to the number of invocations though. Step 2 uses a cached Uni so it does not increment the invocations counter, but the subscriptions counter is still incremented when the Uni is resolved.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see! You cache the Uni and resubscribe to it every time (well Quarkus does).
That's actually interesting, but I'm not sure it's what the user would expect.

Imagine:

@GET
public Uni<String> callMyRemoteService() {
   return webClient.send().map(r -> r.bodyAsString());
}

Basically, it calls a remote service.

If you cache the result, what would/should happen?

  1. the response is cached - the users will get the same response, avoiding calls to the remote service
  2. the uni is cached - to every time there is a request, there is another subscription calling the remote service

I would have said 1, but it looks like 2 has been implemented. Can you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

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

2 happens until one of the subscribers gets the reponse, which will be cached because of the callback at CacheResultInterceptor:116. From there, 1 will happen for all subsequent calls to the cached method. I don't think we can lower the number of remote service calls, but maybe I missed something.

There is one detail that could be changed though: the Uni returned by the cached method does not have to be cached until a response is received. UnresolvedUniValue could act as a placeholder in the cache and be replaced by the reponse when it's available. In the meantime, CacheResultInterceptor.intercept would simply return the method invocation result followed by the CacheResultInterceptor:116 callback.

Copy link
Member Author

@gwenneg gwenneg Apr 19, 2021

Choose a reason for hiding this comment

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

Now that I wrote it down, that placeholder idea sounds more elegant to me than wrapping and then caching the Uni. 😄

If you want, I can add a commit to this PR to show you what I have in mind exactly.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

@gwenneg gwenneg Apr 19, 2021

Choose a reason for hiding this comment

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

I pushed the changes in 2daae56.

UnresolvedUniValue no longer contains the Uni but is still cached as a placeholder until the emitted item is available.

The number of subscriptions is now equal to the number of invocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

If that version is OK with you, I'll push one more commit to make UnresolvedUniValue static instead of creating a new instance for each unresolved Uni.

@gwenneg gwenneg force-pushed the issue-16607-cache-emitted-uni-item branch from 2a577df to 0af0bf3 Compare April 18, 2021 17:06
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 18, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 0af0bf3

Status Name Step Test failures Logs Raw logs
MicroProfile TCKs Tests Verify resteasy-reative dependencies ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 18, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 0af0bf3

Status Name Step Test failures Logs Raw logs
MicroProfile TCKs Tests Verify resteasy-reative dependencies ⚠️ Check → Logs Raw logs

@gwenneg gwenneg force-pushed the issue-16607-cache-emitted-uni-item branch from 0af0bf3 to 85ac88b Compare April 18, 2021 23:00
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 18, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 85ac88b

Status Name Step Test failures Logs Raw logs
MicroProfile TCKs Tests Verify resteasy-reative dependencies ⚠️ Check → Logs Raw logs

@gwenneg gwenneg force-pushed the issue-16607-cache-emitted-uni-item branch from 35d60d7 to 62a28b8 Compare April 19, 2021 09:48
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 19, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 35d60d7

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Reclaim Disk Space ⚠️ Check → Logs Raw logs
CI Sanity Check Build ⚠️ Check → Logs Raw logs
🚫 Devtools Tests - JDK ${{ matrix.java.name }}
🚫 Devtools Tests - JDK 11 Windows
🚫 Gradle Tests - JDK 11 ${{ matrix.os.family }}
🚫 JVM Tests - JDK ${{ matrix.java.name }}
🚫 JVM Tests - JDK 11 Windows
🚫 Maven Tests - JDK ${{ matrix.java.name }}
🚫 Maven Tests - JDK 11 Windows
🚫 MicroProfile TCKs Tests
🚫 Native Tests - ${{ matrix.category }}
🚫 Native Tests - Windows - ${{ matrix.category }}

@gwenneg gwenneg force-pushed the issue-16607-cache-emitted-uni-item branch from 62a28b8 to 2daae56 Compare April 19, 2021 09:55
@quarkus-bot
Copy link

quarkus-bot bot commented Apr 19, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building 62a28b8

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs
🚫 Devtools Tests - JDK ${{ matrix.java.name }}
🚫 Devtools Tests - JDK 11 Windows
🚫 Gradle Tests - JDK 11 ${{ matrix.os.family }}
🚫 JVM Tests - JDK ${{ matrix.java.name }}
🚫 JVM Tests - JDK 11 Windows
🚫 Maven Tests - JDK ${{ matrix.java.name }}
🚫 Maven Tests - JDK 11 Windows
🚫 MicroProfile TCKs Tests
🚫 Native Tests - ${{ matrix.category }}
🚫 Native Tests - Windows - ${{ matrix.category }}

@gwenneg gwenneg force-pushed the issue-16607-cache-emitted-uni-item branch from 2daae56 to 015a3c6 Compare April 19, 2021 12:01
@gwenneg gwenneg force-pushed the issue-16607-cache-emitted-uni-item branch from 015a3c6 to 7662427 Compare April 19, 2021 12:19
@quarkusio quarkusio deleted a comment from quarkus-bot bot Apr 19, 2021
@gwenneg
Copy link
Member Author

gwenneg commented Apr 19, 2021

I pushed the static UnresolvedUniValue instance (it made the code simpler) and squashed the commits. This is ready for a merge.

@gsmet gsmet merged commit 6978921 into quarkusio:main Apr 20, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone Apr 20, 2021
@gwenneg gwenneg deleted the issue-16607-cache-emitted-uni-item branch April 20, 2021 20:01
@gwenneg
Copy link
Member Author

gwenneg commented Apr 20, 2021

Thank you all for your help with this!

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.

Cache the item emitted by a Uni instead of the Uni itself
4 participants