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

More EvictableCache tests #14502

Merged
merged 3 commits into from
Oct 11, 2022
Merged

More EvictableCache tests #14502

merged 3 commits into from
Oct 11, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented Oct 6, 2022

In #14476 (comment) it became apparent we have no tests ensuring no memory leak in EvictableCache. This PR improves test coverage around that.

@findepi findepi added test no-release-notes This pull request does not require release notes entry labels Oct 6, 2022
@cla-bot cla-bot bot added the cla-signed label Oct 6, 2022

assertThat(loads.get())
// At most two loads per key (one before expiration and potential one more after ttl elapsed)
.isLessThanOrEqualTo(6); // TODO should be 4
Copy link
Member Author

Choose a reason for hiding this comment

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

This is being addressed by #14476

cache.getUnchecked(11);
cache.getUnchecked(22);
while (stopwatch.elapsed(MILLISECONDS) <= ttl) {
assertThat(loads.get()).isBetween(2, 6); // TODO should be always 2
Copy link
Member Author

Choose a reason for hiding this comment

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

This is being addressed by #14476

assertEquals(((EvictableCache<?, ?>) cache).tokensCount(), maximumSize);

// Ensure cache is effective, i.e. some entries preserved
int lastKey = 10_000 - 1;
Copy link
Member

Choose a reason for hiding this comment

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

nit: extract 10_000 to variable

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 chose not to apply this one, hope it's fine

Copy link
Member

Choose a reason for hiding this comment

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

Yeah :)

// Ensure cache is effective, i.e. no new load
int lastKey = 10_000 - 1;
assertEquals((Object) cache.get(lastKey), "abc" + lastKey);
assertEquals(loads.get(), 10_000);
Copy link
Member

Choose a reason for hiding this comment

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

extract 10_000

It was recently discovered we lack a test ensuring that time-based cache
has it's entries evicted, and not e.g. using unbounded amount of memory.
@findepi
Copy link
Member Author

findepi commented Oct 10, 2022

AC

ticker.increment(ttl, MILLISECONDS);
// Should be reloaded
assertEquals(cache.get(key, () -> "new value"), "new value");
// TODO (https://github.com/trinodb/trino/issues/14545) tokensCount should be 1; 0 means we lost the token for a live entry
Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #14545

@findepi
Copy link
Member Author

findepi commented Oct 10, 2022

CI #13166, #13633, #14546 :(

@findepi findepi merged commit bdd3132 into trinodb:master Oct 11, 2022
@findepi findepi deleted the findepi/evictly branch October 11, 2022 08:34
@github-actions github-actions bot added this to the 400 milestone Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry test
Development

Successfully merging this pull request may close these issues.

2 participants