-
Notifications
You must be signed in to change notification settings - Fork 300
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
BlobSidecars pruning #7085
BlobSidecars pruning #7085
Conversation
--remaining; | ||
final boolean finished = remaining < 0; | ||
final SlotAndBlockRootAndBlobIndex key = it.next(); | ||
if (finished && (key.isNoBlobsKey() || key.getBlobIndex().equals(ZERO))) { |
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.
So, IIUC, we want to continue pruning blobs for the current slot so we don't partially delete blobs for a given slot.
Correct?
if yes it means that pruned
can go over pruneLimit
and the return statement should be
return pruned >= pruneLimit
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.
Nice catch
} | ||
|
||
@Command( | ||
name = "validate-blobs-sidecars", |
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.
Why you completely removed this command? It was useful to me to check that we have all blobs we expect to have
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.
We'd better to start it from the blank, there are soo many changes in categories
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.
LGTM. Just a nit
|
||
if (blobsSidecarStorageCountersEnabled) { | ||
if (blobSidecarStorageCountersEnabled) { | ||
LabelledGauge labelledGauge = |
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.
We don't need a labelled gague anymore, we could go with metricsSystem.createGauge
now.
OR we can add a label to track also the earliest slot so we can see the availability window moving.
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.
because now we have the total blob counter, which doesn't track directly the number of slots anymore
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.
LGTM
PR Description
BlobSidecar pruning + tests + some extra
Store.retrieveBlobsSidecars...
was removed. We will addStore.retrieveBlobSidecars...
when moving store flow from manager to transactionStore. I propose to add in the same PR replacements for tests:retrieveSignedBlockAndBlobsSidecar_shouldReturnEmptyIfBlockNotPresent
retrieveSignedBlockAndBlobsSidecar_withInconsistentSidecar
retrieveSignedBlockAndBlobsSidecar_withEmptyBlobs
retrieveSignedBlockAndBlobsSidecar_withBlobs
Fixed Issue(s)
Documentation
doc-change-required
label to this PR if updates are required.Changelog