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 new caching logic to all-transactions #2000

Merged
merged 9 commits into from
Apr 30, 2024
Merged

Conversation

moisses89
Copy link
Member

@moisses89 moisses89 commented Apr 18, 2024

Description

  • remove count_all_elements from cache_key
  • use caching by "directories" creating a cache_dir with "all-txs:safe-address"
  • trigger Redis unlink from signal providing the cache_dir to remove all the cache_fields behind the cache_dir.

Untitled Diagram drawio

Closes #1931

@Uxio0
Copy link
Member

Uxio0 commented Apr 18, 2024

No need to use a keccak256 for the key

@moisses89 moisses89 marked this pull request as ready for review April 22, 2024 07:23
@moisses89 moisses89 requested a review from a team as a code owner April 22, 2024 07:23
@moisses89 moisses89 marked this pull request as draft April 22, 2024 08:28
@moisses89 moisses89 marked this pull request as ready for review April 22, 2024 14:38
Copy link
Member

@Uxio0 Uxio0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good work and nice refactor, but I'm missing docs for better understanding this feature, as its really important and critical for the service

safe_transaction_service/history/signals.py Outdated Show resolved Hide resolved
safe_transaction_service/history/signals.py Outdated Show resolved Hide resolved
return (None, None)


def _clean_all_txs_cache(instance):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add docs. Also maybe another python file is better to add these helper methods, as we are merging a lot of things in the signals file. Maybe refactoring all this logic into a class (if possible) could make sense

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I think that we could create a class for all the caching related with endpoints and mange the invalidation there.
We could reuse this caching methods to endpoints like multisig transactions, module transactions.
Issue created here #2008

safe_transaction_service/history/signals.py Outdated Show resolved Hide resolved
safe_transaction_service/history/signals.py Show resolved Hide resolved
safe_transaction_service/history/views.py Outdated Show resolved Hide resolved
@moisses89 moisses89 merged commit 5420d6e into main Apr 30, 2024
6 checks passed
@moisses89 moisses89 deleted the fix_all_transactions_cache branch April 30, 2024 15:06
@github-actions github-actions bot locked and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All-transactions cache can be broken due to deletion of transactions
2 participants