-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Made ShouldPurge protected and virtual in eviction strategy. #6560
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.
Don't see need to alter abstraction or public surfaces. Seems like this can be confined to a implementation detail of this strategy. I think all that is needed is an existing private method be made protected virtual so it can be specialized.
/// <param name="newestCachedMessage"></param> | ||
/// <param name="nowUtc"></param> | ||
/// <returns></returns> | ||
bool ShouldPurge(ref CachedMessage cachedMessage, ref CachedMessage newestCachedMessage, DateTime nowUtc); |
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.
Why add this to the abstraction. Is called by other? Shouldn't currently be, as it was private.
@@ -84,6 +84,16 @@ public void PerformPurge(DateTime nowUtc) | |||
this.periodicMonitoring?.TryAction(nowUtc); | |||
} | |||
|
|||
/// <inheritdoc /> | |||
public virtual bool ShouldPurge(ref CachedMessage cachedMessage, ref CachedMessage newestCachedMessage, DateTime nowUtc) |
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.
Unless it needs to be public, suggest making it a protected virtual. Specializations can override, but still an implementation detail of this strategy.
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.
Okay, will make it protected virtual, somehow that skipped my mind that a "protected virtual" would also work.
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.
LG2M
Will merge once testing finishes.
Good work! Thanks for contributing.
Thank you @adyada ! |
Thanks @jason-bragg for quick review and merging it. |
We are seeing some cases where some items from cache are purged before they get delivered to a grain, as they satisfy the time purge criteria. It could also be because of some customizations we have done to Orleans streaming.
So, just making ShouldPurge as public method so that we can customize it, like only purge cache messages whose sequence token is less than min read sequence token in last 15 mins or something.