-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Feat/graphman/clear stale call cache #6186
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
base: master
Are you sure you want to change the base?
Feat/graphman/clear stale call cache #6186
Conversation
Signed-off-by: Maksim Dimitrov <[email protected]>
Signed-off-by: Maksim Dimitrov <[email protected]>
Signed-off-by: Maksim Dimitrov <[email protected]>
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.
Pull Request Overview
This PR adds functionality to clear stale call cache entries based on TTL (time-to-live) to the graphman
tool. The implementation allows users to remove cached contract calls that haven't been accessed within a specified number of days.
- Adds TTL-based cache clearing with
--ttl-days
option tographman chain call-cache remove
command - Implements batch processing to handle large cache deletions efficiently
- Updates documentation to explain the new cache management options
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
store/postgres/src/chain_store.rs |
Implements the core clear_stale_call_cache method with batch deletion logic for both shared and private storage |
graph/src/components/store/traits.rs |
Adds the trait method definition for TTL-based cache clearing |
node/src/manager/commands/chain.rs |
Adds the command handler for the new TTL functionality |
graph/src/blockchain/mock.rs |
Implements the trait method in the mock store |
store/test-store/tests/postgres/chain_head.rs |
Adds a test to verify the functionality works without errors |
docs/graphman.md |
Updates documentation to describe the new cache management options |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
loop { | ||
let next_batch = cache::table | ||
.select(cache::id) | ||
.filter(cache::contract_address.eq_any(&stale_contracts)) |
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.
Would it be better to batch by contracts rather than cache entries? Im wondering because the number of stale_contracts itself can be very huge, in our production i just checked and the number for >7days is 9624844.
So for each row postgres would need to do a linear scan through this number of address to filter.
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.
If this is this case, probably both have to be batched
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.
Btw is that number in the public eth_call_meta
table?
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.
no its just for ethereum in the private schema
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'll look into adjusting the queries to make them more efficient, but In general I doubt this approach is preferable, even if optimized for such a number of records. Depending on the avg number of call cache records per contract, it could take days to clean up this many.
remove_entire_cache: bool, | ||
/// Remove the cache for contracts that have not been accessed in the last <TTL_DAYS> days | ||
#[clap(long, conflicts_with_all = &["from", "to", "remove-entire-cache"])] | ||
ttl_days: Option<i32>, |
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.
We shouldn't allow negative values here
Another nice feature would be if we can give a small progress bar for the user indicating the deletion progress if its going to take time |
We could print info about each batch. Not sure about progress bars, because that will require fetching the count of all records to be deleted. Also I'm not sure how this will play out later, when the call cache cleaning is implemented as a recurring job, because this file is used by both |
Partialy addresses #4503
What this PR does:
--ttl-days <days>
to thegraphman chain call-cache <chain> remove
commandExecuting the command using the
--ttl-days
option, will remove call cache entries for contracts that haven't been accessed within a user-defined number of days