Factor out different DelayManager implementations #115
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows configurable expiry or refresh if either isn't necessary, without changing the rest of the system.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors delay management by extracting different DelayManager implementations to allow configurable expiry and refresh behavior without affecting the rest of the system.
- Introduces new DelayManager implementations (NoOpDelayManager, ExpiringDelayManager, ExpireAndRefreshDelayManager).
- Replaces the previous concrete DelayedElement and DelayManager with new abstract structures.
- Updates ArcBuilder and Arc to integrate the new delay management design.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/src/main/java/eu/aylett/arc/internal/TimeDelayedElement.java | New implementation using DelayManager.GetDelay, with proper delay calculation. |
| lib/src/main/java/eu/aylett/arc/internal/NoOpDelayedElement.java | Lightweight implementation returning a no-op delay element. |
| lib/src/main/java/eu/aylett/arc/internal/NoOpDelayManager.java | Implements a no-op manager with an empty poll implementation. |
| lib/src/main/java/eu/aylett/arc/internal/ExpiringDelayManager.java | Adds delay elements with expiry handling using a DelayQueue. |
| lib/src/main/java/eu/aylett/arc/internal/ExpireAndRefreshDelayManager.java | Handles both expiry and refresh using two separate DelayQueues. |
| lib/src/main/java/eu/aylett/arc/internal/DelayedElement.java | Refactors DelayedElement to an abstract class, removing previous method implementations. |
| lib/src/main/java/eu/aylett/arc/internal/DelayManager.java | Changes DelayManager to an abstract class, with concrete implementations in subclasses. |
| lib/src/main/java/eu/aylett/arc/ArcBuilder.java | Updates to use DelayManager objects and validates expiry/refresh constraints. |
| lib/src/main/java/eu/aylett/arc/Arc.java | Adjusts constructor to receive a DelayManager, removing explicit duration validations. |
Comments suppressed due to low confidence (1)
lib/src/main/java/eu/aylett/arc/internal/DelayedElement.java:19
- Consider adding class-level documentation for DelayedElement to clearly define the contract expected from its subclasses, now that its previous method implementations have been removed.
public abstract class DelayedElement { }
| @@ -68,6 +76,18 @@ public ArcBuilder withClock(InstantSource clock) { | |||
| int capacity) { | |||
| checkNotNull(loader, "Loader function must be provided"); | |||
| checkArgument(capacity > 0, "Capacity must be greater than zero"); | |||
| return new Arc<>(capacity, loader, pool, expiry, refresh, clock); | |||
|
|
|||
| Verify.verify(expiry != null || refresh == null, "Cannot refresh without expiry"); | |||
There was a problem hiding this comment.
[nitpick] The condition verifying expiry and refresh is somewhat unclear; consider rewriting it to explicitly check that if a refresh duration is provided, then expiry must be non-null, for example by using a clear if statement with an explicit error message.
Suggested change
| Verify.verify(expiry != null || refresh == null, "Cannot refresh without expiry"); | |
| if (refresh != null && expiry == null) { | |
| throw new IllegalArgumentException("Cannot set a refresh duration without first setting an expiry duration."); | |
| } |
Copilot uses AI. Check for mistakes.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This allows configurable expiry or refresh if either isn't necessary, without changing the rest of the system.