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

WIP archival trimming #3876

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Oct 23, 2024

This PR:

  • enforces light node pruning by default (no option to retain samples beyond the sampling window (feature(pruner/light)!: LNs should prune samples beyond availability window by default #3505)
  • moves the concept of availability window to the share/availability package where it belongs and changes the names to be more clear -- light.Window -> availability.RequestWindow ; full.Window -> availability.StorageWindow
  • introduces trimming of Quadrant 4 from archival node storage past the sampling window (removes .q4 files) as default behaviour for archival storage nodes
  • ensures that pruner service can only go from archival --> regular pruning mode by tracking the pruner type in the pruner service's "state" (checkpoint)
  • limits the use of availability.Window to the nodebuilder only as it's the only place where it's necessary (uses time.Duration in other places)
  • no longer uses SamplingWindow implicitly to determine whether pruning is enabled or not in:
    • DASer: SamplingWindow needs to be coupled with a parameter pruningEnabled to actually skip sampling the block --> eventually, in a follow-up PR, this should be delegated to underlying availability implementation and removed entirely from DASer.
    • CORE: Similarly, core pkg needs to know whether the BN is pruned or not to know whether to skip storage entirely, or just to store the ODS, or to store the full ODS + Q4 (if it's a recent block)
    • SHREX: shrex-getter is "hardcoded" in nodebuilder to always construct using the RequestWindow const as it's using the parameter for request routing only
    • PRUNER: all node types - pruned or un-pruned, get a pruner service that will call Prune on the given header heights that are deemed outdated -- whether it actually deletes the entire ODS or just trims the Q4 (depending on the pruning implementation provided)

For the above bullet point, please keep in mind that every component used the SamplingWindow previously in a different way to infer information about whether to perform some action given the header's timestamp or not given a certain SamplingWindow. Since all components that rely on SamplingWindow (listed above) use it slightly differently, it's not really simple to make a function that's generalisable over all of the use cases, so I left it up to the independent components to figure out what to do given the SamplingWindow and whether pruning is enabled or not. Check Next steps section to see how to fix this situation later on.

TODO:

  • make tests less ugly (with path)
  • add "type" of pruning to pruner checkpoint so a node can go from archival -> pruned
  • make sampling window provides more understandable (in nodebuilder)

Next steps (in follow-up PRs):

  • DASer does not need to be aware of SamplingWindow and pruningEnabled --> that should be handled by the respective availability implementation (meaning whether to sample or short-circuit, and what exactly to store from the sampling process)
  • pruner pkg is moved into share pkg bc it deals with storage of shares

@renaynay renaynay added area:storage kind:feat Attached to feature PRs labels Oct 23, 2024
@renaynay renaynay self-assigned this Oct 23, 2024
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2024

Codecov Report

Attention: Patch coverage is 55.96330% with 48 lines in your changes missing coverage. Please review.

Project coverage is 45.35%. Comparing base (2469e7a) to head (87e17d0).
Report is 364 commits behind head on main.

Files with missing lines Patch % Lines
pruner/checkpoint.go 55.88% 15 Missing ⚠️
nodebuilder/das/module.go 0.00% 6 Missing and 1 partial ⚠️
share/availability/window.go 0.00% 7 Missing ⚠️
header/headertest/testing.go 25.00% 5 Missing and 1 partial ⚠️
nodebuilder/pruner/module.go 40.00% 6 Missing ⚠️
das/options.go 20.00% 4 Missing ⚠️
das/daser.go 0.00% 0 Missing and 1 partial ⚠️
nodebuilder/das/constructors.go 0.00% 1 Missing ⚠️
share/shwap/p2p/bitswap/getter.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3876      +/-   ##
==========================================
+ Coverage   44.83%   45.35%   +0.51%     
==========================================
  Files         265      310      +45     
  Lines       14620    21903    +7283     
==========================================
+ Hits         6555     9934    +3379     
- Misses       7313    10912    +3599     
- Partials      752     1057     +305     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:storage kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants