-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add cache updating independently from Indexer #2612
Conversation
Pull Request Test Coverage Report for Build c2073d23-ebfb-4963-b80e-0f62bb5fd7e8
💛 - Coveralls |
2e0eaab
to
79cec7f
Compare
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 am ok with the changes, I just want to point out that I am preparing a PR with another generic cache implementation (I put it on hold but I'm going to fix it now): #2581
Regarding BlockNumber
it has some changes in common with this PR and I'll integrate the ones that are not if this gets merged first.
Also it takes care of the first comment by @ayrat555 (the child_spec
definition)
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.
@saneery I was thinking about this PR and there is a problem to be considered for ordered caches.
The thing is that setting a :ttl_check_interval
is not enough. Their logic is based on an indexing list stored in the cache itself that needs to be updated when the expired element is deleted.
This can be done by setting a :callback
option and I started making it (#2628), but I realized this would add a considerable costs in the case where the cache gets updated by the indexer.
So in the end a change is necessary here, the way I would implement it by invalidating the entire cache after a certain time, starting by changing the macro to only set an expiration on the ids_list_key()
.
Alternatively this could be implemented as an element-based expiration like this:
- add a public function
remove_from_index(key)
to the ordered cache macro (the implementation can be basically what is inside this function I was writing) - add a
:callback
option to both caches to use it whenDISABLE_INDEXER
is used
This however would be less efficient I fear, because the caches are usually filled entirely in one call (when the indexer is not used)
What do you think?
I don't know if my explanation is clear enough, if you want I can add a commit to this PR myself.
@pasqu4le please feel free |
@saneery sorry if I kept you waiting. I pushed 2 commits on top of this branch, the first one solves the issue with removing the expired elements from the caches and the second one fixes my comment about merging options from the application environment. I kept them separated because I did not know what your answer was regarding the second one. |
cf8820e
to
00717e8
Compare
00717e8
to
c856a59
Compare
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.
It works fine now 👍 with disabled indexer
Relates to #2583
Motivation
Blocks
,Transactions
andBlockNumber
caches are updated by Indexer, but when Indexer runs separately from WebApp, caches aren't updated. So I added TTL for this caches to updating them without Indexer.Checklist for your PR
CHANGELOG.md
with this PRmaster
in theVersion
column.