Fix `EvictableCacheBuilder#expireAfterWrite` behavior by atanasenko · Pull Request #14476 · trinodb/trino

atanasenko · 2022-10-05T13:42:25Z

Description

When .expireAfterWrite() is set, EvictableCache loads the value twice
after expiration. On any read/write operation the cache might call a
maintenance loop which might evict expired items. Evicted items might
include arbitrary set of values, including the one that is currently
being read, in this case a reload of the value will follow eviction.

With this change key tokens for evicted items are not removed on
notification, but instead only marked as expired, while at the end of
operation the currently processed token is restored while all others are
removed.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

Fixes #14545

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

findepi · 2022-10-05T15:26:31Z

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

Will this make this test flaky when there is a GC pause?

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCache.java

atanasenko · 2022-10-06T12:28:44Z

There is still the #asMap() method which returns a live map operations of which might also cause evictions but those are not handled properly. What do you think about setting an additional flag when any of the values are expired, and if the flag is set, force the removal of tokens on access to tokens map instead of doing it after the operation?

atanasenko · 2022-10-06T12:55:05Z

Actually, disregard that, I went with a simple approach where evicted tokens for the currently loaded value get revived and stored in the map again. There is still a chance that a parallel thread will see the missing token and reload the value, but I think this is acceptable.

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

findepi · 2022-10-06T13:53:37Z

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

Use Stopwatch -- it's ligheter-weight and easier to lead.

BTW i wanted to put a PR with a test preventing memory leak (#14476 (comment)) and end-up also writing my version of this test #14502. not sure if this helps

findepi · 2022-10-06T13:55:09Z

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

the timeout until.isAfter(Instant.now()) condition is checked before a call is made, so we should expect allow one more call here

It never goes past the threshold of the last expiration. Or am I missing something?

You don't know how much time elapses between checking the clock and invoking cache.get

am i missing something?

i think this boils down to the fact it's much better to use ~Ticker for tests, per @losipiuk #14502 (comment)

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCache.java

When `.expireAfterWrite()` is set, EvictableCache loads the value twice after expiration. On any read/write operation the cache might call a maintenance loop which might evict expired items. Evicted items might include arbitrary set of values, including the one that is currently being read, in this case a reload of the value will follow eviction. With this change tokens that got expired are revived before loading them again.

atanasenko · 2022-10-06T16:43:26Z

I moved the 'revival' deeper into the loading of the value. It turns out loading is happening after the eviction of expired items, an attempt to read as well as the removal notifications calls.

findepi · 2022-10-07T13:36:32Z

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCache.java

        Token<K> token = tokens.computeIfAbsent(key, ignored -> newToken);
+        Callable<? extends V> valueLoaderImpl = () -> {
+            // revive token if it got expired before reloading
+            tokens.computeIfAbsent(token.getKey(), ignored -> token);


would this be equivalent?

Suggested change

tokens.computeIfAbsent(token.getKey(), ignored -> token);

tokens.computeIfAbsent(key, ignored -> token);

findepi · 2022-10-07T13:37:46Z

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCache.java

+        Callable<? extends V> valueLoaderImpl = () -> {
+            // revive token if it got expired before reloading
+            tokens.computeIfAbsent(token.getKey(), ignored -> token);
+            return valueLoader.call();


just thinking -- should the revival happen before or after valueLoader.call?
or it doesn't matter?

findepi · 2022-10-07T13:38:55Z

lib/trino-collect/src/test/java/io/trino/collect/cache/TestEvictableCache.java

am i missing something?

findepi · 2022-10-07T13:40:07Z

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCache.java

+        Callable<? extends V> valueLoaderImpl = () -> {
+            // revive token if it got expired before reloading
+            tokens.computeIfAbsent(token.getKey(), ignored -> token);
+            return valueLoader.call();


related -- if valueLoader throws and this wasn't a new token, we can revive it and leak

findepi · 2022-10-14T14:13:05Z

I tried addressing it slightly differently #14644

atanasenko · 2022-10-17T14:44:29Z

Superseded by #14644

atanasenko requested a review from findepi October 5, 2022 13:42

cla-bot bot added the cla-signed label Oct 5, 2022

findepi requested changes Oct 5, 2022

View reviewed changes

atanasenko force-pushed the cache_expire_after_write branch from 915b59c to 07e67c3 Compare October 6, 2022 12:00

atanasenko requested a review from findepi October 6, 2022 12:00

ksobolew reviewed Oct 6, 2022

View reviewed changes

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCache.java Outdated Show resolved Hide resolved

lib/trino-collect/src/main/java/io/trino/collect/cache/EvictableCache.java Outdated Show resolved Hide resolved

atanasenko force-pushed the cache_expire_after_write branch 2 times, most recently from c24d3ff to a4d5430 Compare October 6, 2022 12:23

atanasenko force-pushed the cache_expire_after_write branch from a4d5430 to 033009e Compare October 6, 2022 12:52

findepi mentioned this pull request Oct 6, 2022

More EvictableCache tests #14502

Merged

findepi reviewed Oct 6, 2022

View reviewed changes

atanasenko force-pushed the cache_expire_after_write branch 2 times, most recently from 1433f14 to c331728 Compare October 6, 2022 16:21

atanasenko force-pushed the cache_expire_after_write branch from c331728 to 12f5314 Compare October 6, 2022 16:38

atanasenko requested a review from findepi October 6, 2022 16:44

findepi reviewed Oct 7, 2022

View reviewed changes

findinpath self-requested a review October 14, 2022 13:10

findepi mentioned this pull request Oct 14, 2022

Load value once after expiration in EvictableCache #14644

Merged

atanasenko closed this Oct 17, 2022

	tokens.computeIfAbsent(token.getKey(), ignored -> token);
	tokens.computeIfAbsent(key, ignored -> token);

Conversation

atanasenko commented Oct 5, 2022 • edited by findepi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atanasenko commented Oct 6, 2022

Uh oh!

atanasenko commented Oct 6, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atanasenko commented Oct 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Oct 14, 2022

Uh oh!

atanasenko commented Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

atanasenko commented Oct 5, 2022 •

edited by findepi

Loading