-
-
Notifications
You must be signed in to change notification settings - Fork 289
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: allow blob archival for bigger time periods #6393
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6393 +/- ##
============================================
- Coverage 60.14% 59.38% -0.76%
============================================
Files 407 388 -19
Lines 46485 44802 -1683
Branches 1545 1396 -149
============================================
- Hits 27957 26607 -1350
+ Misses 18496 18165 -331
+ Partials 32 30 -2 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
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.
Makes sense! LGTM!! 🚀
Would anyone ever want to add a way to prune the DB on restart if the consumer wants the flag to go away? I suppose just deleting the db is fine but figured it was worth asking just in case.
logger.verbose(`blobSidecars prune: no entries before epoch ${blobSidecarsMinEpoch}`); | ||
if (archiveBlobEpochs !== Infinity) { | ||
const blobsArchiveWindow = Math.max(config.MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS, archiveBlobEpochs ?? 0); | ||
const blobSidecarsMinEpoch = currentEpoch - blobsArchiveWindow; |
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 guess we are fine with this being a negative number, in that case, do we even need to handle Infinity
explicitly?
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.
Thats a good point. Would just get skipped as normal, although the log output would be funny...
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.
although the log output would be funny
It is not that bad
blobSidecars prune: no entries before epoch -Infinity
but I guess the other log is more explicit about it
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.
its good to be explicit not just for code correctness but also because for code readability and understanding which is one of the lodestar's objective (to stay readable and declarative)
🎉 This PR is included in v1.16.0 🎉 |
a feature requested by EF devops to archive blobs for longer periods