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

Implement reload cache clearing for CloudRequestCache #12526

Merged
merged 16 commits into from
Mar 22, 2025

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Mar 17, 2025

Pull Request Description

This also refactors ReloadDetector to reduce the boilerplate needed to implement reload-triggered cache clearing.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@GregoryTravis GregoryTravis marked this pull request as ready for review March 17, 2025 21:07
@@ -61,6 +69,8 @@ public void cleanExpiredEntries() {
}

public void put(String key, Value value, Duration ttl) {
ReloadDetector.INSTANCE.clearOnReloadIfRegistered(this);
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if it could be worth it to encapsulate this piece of code into a method, so that we keep it in one place in case the method call pattern changes. But perhaps unnecessary.

private void checkReload() {
  ReloadDetector.INSTANCE.clearOnReloadIfRegistered(this);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've simplified this by making ReloadDetector static and removing INSTANCE, since it's not necessary.

@@ -84,6 +81,7 @@ public LRUCache(LRUCacheSettings settings, NowGetter nowGetter, DiskSpaceGetter
this.settings = settings;
this.nowGetter = nowGetter;
this.diskSpaceGetter = diskSpaceGetter;
ReloadDetector.INSTANCE.register(this);
Copy link
Member

Choose a reason for hiding this comment

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

The APIRequestCache seems to not have this register call. Is that by design or omission?

Copy link
Member

Choose a reason for hiding this comment

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

Oh now I noticed - the base class does not have it but the implementation CloudRequestCache does. Ok.

I'd suggest to add a note in the APIRequestCache doc comment mentioning that implementations should call register.

Or perhaps couldn't we make the APIRequestCache call it in its constructor? So that implementations call it automatically. I'm worried someone creating a new implementation may easily forget this call so ideally I'd like to structure the code in such a way that it is hard to forget it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to allow subclasses to decide if they want to use ReloadDetector, so I've just added some documentation to APIRequestCache.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I thought that it would fail with the 'not registered exception' but didn't notice you have clearOnReloadIfNotRegistered so it checks the registration.

Makes sense now, thanks for clarifying! And the note in the docs is appreciated.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Overall changes look very good.

Just one nitpick if we can try to make APIRequestCache easier to not-forget registering. But feel free to ignore if not as easy as I thought.

@github-actions github-actions bot added the -libs-API-change-Base Marks a PR that changes the public API of Standard.Base label Mar 18, 2025
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Looks alright minus some potential race-conditions

@GregoryTravis GregoryTravis added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Mar 19, 2025
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Mar 22, 2025
@mergify mergify bot merged commit 94d48da into develop Mar 22, 2025
67 of 68 checks passed
@mergify mergify bot deleted the wip/gmt/11762-clear-cloud-cache branch March 22, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-libs-API-change-Base Marks a PR that changes the public API of Standard.Base CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants