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

pruner: Implement skeleton for pruner package #2972

Merged
merged 5 commits into from
Dec 5, 2023

Conversation

renaynay
Copy link
Member

This PR implements a skeleton for the pruner package and integrates the pruner's AvailabilityWindow determination into the DASer sampling routine.

Namings are up for discussion.

@renaynay renaynay added area:storage kind:feat Attached to feature PRs labels Nov 28, 2023
pruner/pruner.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

i preferred when you named this a carcass, but skeleton makes sense :-)

Really my only genuine comments are that i'm stuck on naming, i wonder if the package should be called storage or something? Thinking out loud on naming, but maybe it's:

  • storage (ie: concerns related to storage of blocks)
  • storage/pruner = where pruner implements the various 'workers' implementations that performs pruning for diff node types
  • storage/availability (which then implements a range of Window functions or Sampling strategies for node types)

Thinking then it conceptually reads like

Daser, is injected with an availabilityWindowSampler, which checks sample/storage conditions based on the header coming in, and then storage has pruner package "pruning" which implements various strategies for pruning data which is adjacent to windowSampler.

Thinking this is not dissimilar to OTEL and trace sampling etc

is there a better name than storage as top level if we went with that?

so then pruning.AvailabilityWindow -> availability.WindowSampler, and maybe has sampler.ShouldSample(h Header) true etc?

all suggestions. also, reading about pruning so much makes me think of baby food.

nodebuilder/prune/module.go Show resolved Hide resolved
pruner/pruner.go Outdated Show resolved Hide resolved
das/daser_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (24b333d) 50.92% compared to head (3a56040) 50.94%.
Report is 5 commits behind head on main.

Files Patch % Lines
das/daser.go 63.63% 3 Missing and 1 partial ⚠️
das/options.go 0.00% 4 Missing ⚠️
nodebuilder/das/constructors.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2972      +/-   ##
==========================================
+ Coverage   50.92%   50.94%   +0.01%     
==========================================
  Files         168      168              
  Lines       10944    11038      +94     
==========================================
+ Hits         5573     5623      +50     
- Misses       4872     4913      +41     
- Partials      499      502       +3     

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

pruner/pruner.go Outdated Show resolved Hide resolved
pruner/window.go Outdated Show resolved Hide resolved
pruner/factory.go Outdated Show resolved Hide resolved
pruner/light/prune.go Outdated Show resolved Hide resolved
pruner/pruner.go Outdated Show resolved Hide resolved
pruner/pruner.go Outdated Show resolved Hide resolved
pruner/factory.go Outdated Show resolved Hide resolved
pruner/factory.go Outdated Show resolved Hide resolved
pruner/pruner.go Outdated Show resolved Hide resolved
@renaynay
Copy link
Member Author

renaynay commented Dec 4, 2023

@Wondertan @walldiss @ramin

I've removed IsWithinAvailabilityWindow from the Pruner interface and it is now being supplied to fx via a const that is defined per implementation, where archival provides time.Duration of 0 (disables sampling window calculation) and light provides a time.Duration of 30 days worth of seconds. The full implementation (whenever it is implemented) will be parameter-based as that is the only pruning implementation that is allowed to be configurable (as long as it is minimum 30 days worth of seconds).

How do we feel about this approach?

Benefits: it does not break config for DASer and it only would introduce a config where actually necessary/allowable (full-pruning).

@renaynay
Copy link
Member Author

renaynay commented Dec 4, 2023

Another benefit - enabling sampling window for light nodes will be as simple as changing this one line: https://github.com/celestiaorg/celestia-node/pull/2972/files#diff-67d0619a422b8d0b0254a8f10165ae1d47bcc142b7fbd3e61591262f448731baR42

Copy link
Contributor

@ramin ramin left a comment

Choose a reason for hiding this comment

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

kinda love this tbh

Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

Looks great 🚀

pruner/light/window.go Show resolved Hide resolved
@renaynay renaynay enabled auto-merge (squash) December 5, 2023 16:46
@renaynay renaynay merged commit dc1bd00 into celestiaorg:main Dec 5, 2023
25 of 34 checks passed
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.

6 participants