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

Create file system based BlobSidecar data archive #8674

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

david-ry4n
Copy link
Contributor

@david-ry4n david-ry4n commented Oct 3, 2024

PR Description

Follows on from #8640 to allow pruned BlobSidecars to be written to the file system as an archive.
Initial implementation requires a base path where BlobSidecars are written to a directory structure as
basePath/ab/cd/abcd... where the hex string of the block root starts with abcd. The slot and block root
are also written to index.dat in the basePath directory.

Fixed Issue(s)

partially address #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.

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.

Looks nice! some comments at first pass

@@ -170,6 +170,16 @@ public class BeaconNodeDataOptions extends ValidatorClientDataOptions {
arity = "0..1")
private int blobsPruningLimit = StorageConfiguration.DEFAULT_BLOBS_PRUNING_LIMIT;

@CommandLine.Option(
names = {"--Xdata-storage-archive-path"},
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 add blob to the name of this flag to make it explicit that it is only a functionality that archive blobs rather than general (which gives the impression that it could store blocks as well as other things like state...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I think you're looking at an older version. I did update this to "Xdata-storage-blobs-archive-path" to align with the other blobs options. :)

@david-ry4n david-ry4n requested a review from tbenr October 11, 2024 04:33
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.

Nice! Just nits and a couple of questions

final DataArchive dataArchive =
config
.getBlobsArchivePath()
.map(path -> (DataArchive) new FileSystemArchive(Path.of(path)))
Copy link
Contributor

Choose a reason for hiding this comment

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

you can avoid cast via
.<DataArchive>map(path -> new FileSystemArchive(Path.of(path)))

import tech.pegasys.teku.spec.datastructures.blobs.versions.deneb.BlobSidecar;

/**
* Interface for a data archive which stores non-current BlobSidecars and could be extended later to
Copy link
Contributor

Choose a reason for hiding this comment

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

non-current, maybe something more specific like prunable or BlobSidecars outside the Data Availability window


// Just warn if we failed to find all the BlobSidecars.
if (keys.size() != blobSidecars.size()) {
LOG.warn("Failed to retrieve BlobSidecars for keys:{}", keys);
Copy link
Contributor

Choose a reason for hiding this comment

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

missing space keys: {}

if (!blobSidecarArchived) {
LOG.warn("Failed to archive BlobSidecar for slot:{}. Stopping pruning", key.getSlot());
LOG.warn("Failed to archive and prune BlobSidecars. Stopping pruning");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is an error. It requires an action from the user (like checking diskspace). We could also ask user to do something

logPruningResult(
"Non canonical Blobs pruning finished in {} ms. Limit reached: {}",
nonCanonicalBlobsPruningStart,
nonCanonicalBlobsLimitReached);
}
} catch (ShuttingDownException | RejectedExecutionException ex) {
} catch (ShuttingDownException | RejectedExecutionException | IOException ex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

IOException -> shutting down?

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.

3 participants