-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Serialized storage cache #3669
base: master
Are you sure you want to change the base?
Serialized storage cache #3669
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.
Good, it should work this way. But I'd suggest to process existing chains with this change in order to ensure that cache persisting logic works as expected.
/// <summary> | ||
/// Serialized cache | ||
/// </summary> | ||
SerializedCache SerializedCache { get; } |
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.
Adding this to the IReadOnlyStore
makes the interface more complex, and each implementation needs to be modified.
Another way:
Add a wrapper class for IStore
class SerializedCacheStore {
private readonly IStore _store,
public SerializedCache SerializedCache { get; } = new();
public(IStore store) { _store = store}
}
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.
And change all the code where we define it?
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.
@shargon
why not?
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.
I think is easier and simpler like this, Why we need two different types, one with cache and other without?
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.
If you create an additional class called SerializedCache
, doesn't require IReadOnlyStore
. Also its misleading and creates more problems than it solves. For example: var cache = Store.SerializedCache.SerializedCache.SerializedCache.SerializedCache.SerializedCache.SerializedCache
is that intended?
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.
SerializedCache
is not IReadOnlyStore
so you can't do Store.SerializedCache.SerializedCache.SerializedCache.SerializedCache.SerializedCache.SerializedCache
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.
Than it needs to inherit from that interface. All other cache classes do. Now your limiting the class.
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.
I don't understand you, you can't do it Store.SerializedCache.SerializedCache.SerializedCache.SerializedCache.SerializedCache.SerializedCache
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.
I think is easier and simpler like this, Why we need two different types, one with cache and other without?
Multi-layer structure.
One layer one feature.
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 needed to cast something that always will be this type, waste of time, when we want to use it we will need to cast the type, and always we will create this type, otherwise we can't use it, for other things is ok, for this use case, seems worthless to me
Description
Close #3145
Alternative to #3146
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: