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 generic Ordered Cache behaviour and implementation #2497

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

pasqu4le
Copy link
Contributor

@pasqu4le pasqu4le commented Aug 2, 2019

Motivation

Part of #2427

Multiple caches exist to keep a page of the most recent n elements.
More are to be implemented, so it would be good to abstract their implementation.

Changelog

Enhancements

This PR adds a behaviour and a macro (providing an implementation) to create this type of caches.
It also redefines the existing ones using the new macro.

Checklist for your PR

Note: I did not write tests for the new module. however the existing tests for the transactions and blocks caches should cover the most important cases

@pasqu4le pasqu4le self-assigned this Aug 2, 2019
@pasqu4le pasqu4le force-pushed the pp-general-ordered-cache branch from f9efd1e to 050ec55 Compare August 2, 2019 17:45
@pasqu4le pasqu4le added ready for review This PR is ready for reviews. and removed in progress labels Aug 2, 2019
@coveralls
Copy link

coveralls commented Aug 2, 2019

Pull Request Test Coverage Report for Build 2993a38b-976b-44f3-a470-85e738b27086

  • 14 of 22 (63.64%) changed or added relevant lines in 7 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.3%) to 79.156%

Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/explorer/lib/explorer/chain.ex 4 5 80.0%
apps/explorer/lib/explorer/chain/ordered_cache.ex 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
apps/block_scout_web/lib/block_scout_web/controllers/chain/market_history_chart_controller.ex 2 71.43%
apps/explorer/lib/explorer/chain/cache/transaction_count.ex 2 84.21%
Totals Coverage Status
Change from base Build 9b8975aa-67e5-4419-90f4-0d509483294b: -0.3%
Covered Lines: 5157
Relevant Lines: 6515

💛 - Coveralls

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.

@pasqu4le I installed this cache generalization to stg server and the issue is that after a while the price and marketcap charts disappear together with the values at the bottom of charts. I am not sure that exactly this PR caused this problem, but I guess it could be since we recently implemented a cache for charts data #2035. You can reproduce the behaviour in the stg server.

Screenshot 2019-08-07 at 19 35 06

@pasqu4le
Copy link
Contributor Author

pasqu4le commented Aug 8, 2019

@vbaranov I don't how it's possible for this PR to be the cause.
It cannot have caused the problem directly, because this is a generic cache macro only for ordered lists caches, only the blocks and transactions ones have been re-implemented with it.

Also indirectly there does not seem to be any correlation.

P.s. It's probably a good idea to write a general macro for this type of caches that store only a finite number of "points". With it and the one in this PR we'd probably cover them all, I'll work on it and make a separate PR.

@pasqu4le pasqu4le force-pushed the pp-general-ordered-cache branch from 050ec55 to 077b51c Compare August 9, 2019 14:33
@vbaranov vbaranov self-requested a review August 13, 2019 16:25
@vbaranov
Copy link
Member

The problem found was caused by #2473 update that fixed #2550

@vbaranov
Copy link
Member

@pasqu4le could you please fix credo test?

Problem: multiple caches exist to keep a page of the most recent `n` elements.
More are to be implemented, so it would be good to abstract their implementation.

Solution: add a behaviour and a macro (providing an implementation) to create this type of caches.
Redefine the existing ones using the new macro.
@pasqu4le pasqu4le force-pushed the pp-general-ordered-cache branch from a17dfed to b52d10b Compare August 14, 2019 14:54
@pasqu4le
Copy link
Contributor Author

@vbaranov sorry about that, they are fixed now

@vbaranov vbaranov merged commit 899ab8c into master Aug 14, 2019
@vbaranov vbaranov deleted the pp-general-ordered-cache branch August 14, 2019 15:02
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