Skip to content
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

Merged
merged 2 commits into from
Sep 5, 2019

Conversation

saneery
Copy link
Contributor

@saneery saneery commented Aug 21, 2019

Relates to #2583

Motivation

Blocks, Transactions and BlockNumber 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

@coveralls
Copy link

coveralls commented Aug 21, 2019

Pull Request Test Coverage Report for Build c2073d23-ebfb-4963-b80e-0f62bb5fd7e8

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.637%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/explorer/lib/explorer/chain/ordered_cache.ex 0 1 0.0%
Totals Coverage Status
Change from base Build 40fa414a-f7e7-4e58-b60a-3f6392fa16cc: 0.0%
Covered Lines: 5146
Relevant Lines: 6544

💛 - Coveralls

@saneery saneery force-pushed the caches-without-indexer branch from 2e0eaab to 79cec7f Compare August 21, 2019 14:17
@saneery saneery requested review from pasqu4le, ayrat555 and vbaranov and removed request for pasqu4le and ayrat555 August 21, 2019 14:18
Copy link
Contributor

@pasqu4le pasqu4le left a 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)

@pasqu4le pasqu4le self-requested a review August 27, 2019 07:27
Copy link
Contributor

@pasqu4le pasqu4le left a 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 when DISABLE_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.

@saneery
Copy link
Contributor Author

saneery commented Aug 30, 2019

if you want I can add a commit to this PR myself.

@pasqu4le please feel free

@pasqu4le
Copy link
Contributor

pasqu4le commented Sep 2, 2019

@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.

@pasqu4le pasqu4le force-pushed the caches-without-indexer branch from 00717e8 to c856a59 Compare September 3, 2019 13:02
@pasqu4le
Copy link
Contributor

pasqu4le commented Sep 3, 2019

@vbaranov @ayrat555 I have rebased/updated/fixed the PR and it should now be ready to be used. Please review it when you can.

@pasqu4le pasqu4le added the ready for review This PR is ready for reviews. label Sep 3, 2019
Copy link
Member

@vbaranov vbaranov left a 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

@vbaranov vbaranov merged commit 2432171 into master Sep 5, 2019
@vbaranov vbaranov deleted the caches-without-indexer branch September 5, 2019 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review This PR is ready for reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants