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

feat(share/availability/full): Introduce Q4 trimming for archival nodes #4028

Merged
merged 8 commits into from
Jan 20, 2025

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Jan 5, 2025

This PR introduces trimming of .q4 files outside the sampling window for archival nodes.

It also updates the pruner checkpoint such that it's possible to convert from an archival node --> full pruning node but not the other way around (the node runner would need to resync the node in this case).

All deletion logic now lives in either share/availability/light or share/availability/full but the pruner service is still contained in a separate package and now runs by default as all node types will have some form of pruning happening.

@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2025

Codecov Report

Attention: Patch coverage is 37.50000% with 75 lines in your changes missing coverage. Please review.

Project coverage is 45.09%. Comparing base (2469e7a) to head (3e1bb92).
Report is 417 commits behind head on main.

Files with missing lines Patch % Lines
nodebuilder/pruner/module.go 0.00% 47 Missing ⚠️
nodebuilder/pruner/migration_utils.go 69.23% 6 Missing and 2 partials ⚠️
share/availability/full/availability.go 0.00% 7 Missing ⚠️
share/availability/full/utils.go 63.15% 5 Missing and 2 partials ⚠️
pruner/service.go 33.33% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4028      +/-   ##
==========================================
+ Coverage   44.83%   45.09%   +0.25%     
==========================================
  Files         265      309      +44     
  Lines       14620    22326    +7706     
==========================================
+ Hits         6555    10067    +3512     
- Misses       7313    11185    +3872     
- Partials      752     1074     +322     

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

pruner/pruner.go Outdated Show resolved Hide resolved
@renaynay renaynay added the kind:feat Attached to feature PRs label Jan 5, 2025
nodebuilder/pruner/module.go Outdated Show resolved Hide resolved
pruner/checkpoint.go Outdated Show resolved Hide resolved
@renaynay renaynay changed the title WIP - introduce archival trimming feat(share/availability/full): Introduce Q4 trimming for archival nodes Jan 7, 2025
@renaynay renaynay marked this pull request as ready for review January 9, 2025 14:50
pruner/checkpoint.go Outdated Show resolved Hide resolved
@renaynay
Copy link
Member Author

There's currently an annoying edge-case to the solution presented in 6087e64 that wouldn't be able to detect a node that has been previously run with pruner enabled (on older version where archival trimming was not present), and once upgraded to new version, runs without pruning enabled successfully even though it should not be allowed to revert to an archival run.

I need to figure out how to prevent this without making a super ugly solution.

information about the nodes previous run (archival or pruned) and is
checked during startup
@renaynay
Copy link
Member Author

The solution to the above comment ^ is f370346

@walldiss
Copy link
Member

Looks good. It took me a while to understand migration. I know it is temporary code, so not insisting to change. Perhaps it might be simpler if migration was detected inside nodebuilder? This way It will be easier to remove later, wihtout the need for refactor. Smth like this:

	// nodebuilder/share
	migration(isarhival bool){
		// Should check internaly prunned <-> archival migration of new endpoints
		created, err := avail.CreateCheckpoint(isarhival)
		
		// temporary method, can be removed later with migration() func
		wasPrunned := pruner.HasCheckpoint()
		
		if isarhival && wasPrunned && !created{
			// unwanted migration detected
		}
	}

Another thing to consider is lack of archival -> pruned conversion support in the PR. We should reset prunner checkpoint to allow it to Remove Q1 files.

cristaloleg
cristaloleg previously approved these changes Jan 16, 2025
Copy link
Contributor

@cristaloleg cristaloleg left a comment

Choose a reason for hiding this comment

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

SGTM

@renaynay
Copy link
Member Author

@walldiss

re #4028 (comment)

I can move the logic to nodebuilder, yes.

walldiss
walldiss previously approved these changes Jan 17, 2025
Copy link
Member

@walldiss walldiss left a comment

Choose a reason for hiding this comment

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

Thank you for dedication for making it better! Happy that we have temporary logic extracted to migration_utils.go. It has its flaws of reimplementing logic for storing previous run, but I don't think it is a big deal, since it will be remove relatively soon.

Added Few optional nits, otherwise Looks good to me!

pruner/service.go Outdated Show resolved Hide resolved
nodebuilder/pruner/module.go Show resolved Hide resolved
share/availability/full/utils.go Show resolved Hide resolved
walldiss
walldiss previously approved these changes Jan 17, 2025
vgonkivs
vgonkivs previously approved these changes Jan 17, 2025
@renaynay renaynay dismissed stale reviews from vgonkivs and walldiss via b4b492f January 17, 2025 20:48
@renaynay renaynay enabled auto-merge (squash) January 20, 2025 09:42
@renaynay renaynay merged commit a700939 into celestiaorg:main Jan 20, 2025
22 of 23 checks passed
@renaynay renaynay deleted the archival-trim branch January 20, 2025 09:51
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.

5 participants