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

Implement a DatabaseArchiveWriter and add to pruneOldestBlobSidecars #8640

Merged

Conversation

david-ry4n
Copy link
Contributor

@david-ry4n david-ry4n commented Sep 24, 2024

PR Description

Initial change to add the concept of DatabaseArchiveWriter to pruneOldestBlobSidecars method. The initial implementation is a NoOp that returns true. See #8513 for additional tasks. With this change it sets up the structure. Future changes will implement a DatabaseArchive interface and required configuration options to configure where/how the archive is written.

Fixed Issue(s)

partially addresses #8513

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

… Currently not configurable and only implements a NoopWriter.
@tbenr
Copy link
Contributor

tbenr commented Sep 24, 2024

welcome to MethodInputParametersMustBeFinal world 😄

Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM. just a nit

@@ -81,6 +85,9 @@ public BlobSidecarPruner(
this.pruningActiveLabelledGauge = pruningActiveLabelledGauge;
this.storeNonCanonicalBlobSidecars = storeNonCanonicalBlobSidecars;

// To be updated with other implementations. e.g. filesystem or s3
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use // TODO: prefix so it will be captured nicely by intellij, or simply remove since we have a (set of) issues on the topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix that up in the next PR. Thanks for feedback.

@david-ry4n david-ry4n merged commit a4e4fcc into Consensys:master Sep 25, 2024
17 checks passed
@david-ry4n david-ry4n deleted the blobsidecar-archive-writer-noop branch October 21, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants