-
-
Notifications
You must be signed in to change notification settings - Fork 20
GH-1153 Rework delay system with per-entry Instant TTL #1184
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the delay system to use per-entry TTLs, which is a significant improvement for avoiding cache-level conflicts. The introduction of DefaultDelay
and ExplicitDelay
interfaces, along with a factory, creates a much cleaner and safer API. My review highlights a critical issue where expireAfterWrite
was unintentionally reintroduced in GuavaDefaultDelay
, undermining the primary goal of this refactoring. I have also included some suggestions to improve the API design by reducing code duplication and better aligning the interface documentation with its behavior, which would enhance flexibility and maintainability.
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDefaultDelay.java
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/DefaultDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/DefaultDelay.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job i like it but i'm thinking about overenginering at this case - maybe Im wrong
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request provides a solid refactoring of the delay system, moving from a cache-level TTL to a per-entry TTL managed by Instant
. This is a great improvement for correctness and prevents premature evictions. The introduction of DefaultDelay
and ExplicitDelay
interfaces, along with a factory class, results in a much cleaner and more robust API. The implementation is well-structured, especially the shared logic in GuavaDelay
. I've identified a few minor inconsistencies in handling expiration edge cases that could be addressed to further improve the robustness of the new system. Overall, this is a high-quality contribution.
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDefaultDelay.java
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDelay.java
Outdated
Show resolved
Hide resolved
eternalcore-api/src/main/java/com/eternalcode/core/delay/GuavaDelay.java
Outdated
Show resolved
Hide resolved
…Delay.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…Delay.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
eternalcore-api/src/main/java/com/eternalcode/core/delay/Delay.java
Outdated
Show resolved
Hide resolved
….java Co-authored-by: Igor Michalski <imichalsky00@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid to me. Shout-out for the good documentation of the API.
* | ||
* @param <T> key type | ||
*/ | ||
public interface DefaultDelay<T> extends ExplicitDelay<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to FixedDelay. Sounds better imo
Description
This PR refactors the delay handling to eliminate cache-level TTL conflicts.
expireAfterWrite
to ensure TTL is controlled per entry via InstantResult: cleaner API, safer usage, no premature cache evictions.
Fixes #1153
Type of change
How Has This Been Tested?