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.
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
[Off-chain] feat: in-memory query cache(s) #994
base: main
Are you sure you want to change the base?
[Off-chain] feat: in-memory query cache(s) #994
Changes from 4 commits
830a7f6
b0f08c3
8743246
d895441
23cc94a
366ab1d
4632c74
a467428
e48a2f2
d5ce62f
71da7f1
6774d3a
086d219
d6dbc44
0414e9a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What if we delete the (key,value) pair for
latestHeight
?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.
Could you elaborate? I'm not quite following.
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.
Yea, this comment doesn't make sense if you don't know where I was looking or what I was thinking...
If we delete the key for the latest height, do we need to also make an appropriate call to
c.latestHeight.Store(??)
?If this is indeed the case, please add a unit test for it.
If not, would just appreciate a quick (no need for anything big) explanation.
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.
Gotcha, good question. I don't think we need to update
c.latestHeight
, regardless of which key is being deleted. ForGet
s, this should be completely transparent to the caller; forSet
s, it means that if you callDelete
, followed bySet
(as opposed toSetAtHeight
), then the height you're setting will be the lastc.latestHeight
. I think this behavior is correct.I did see 👆 but had to think it through for myself anyways:
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.
Given the explanation and diagram, I don't fully understand the need for
latestHeight
at all. In fact, I think we should delete it now if the behaviour you described is expected.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 failed to reconcile with this thread but after discussing with @red-0ne, we decided that it should be an error to call
Set()
on aHistoricalQueryCache
. I think this indicates thatHistoricalQueryCache
SHOULD NOT embedQueryCache
after all. Subsequently,latestVersion
is no longer necessary and can be removed.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.
(class diagrams also updated)