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

Reuse earliest blob slot #9031

Merged
merged 21 commits into from
Jan 29, 2025
Merged

Conversation

gfukushima
Copy link
Contributor

@gfukushima gfukushima commented Jan 24, 2025

PR Description

Based on top of ##9029
This uses similar approach of the earliest block available. Reuse the variable value when present otherwise keep the current behaviour of creating a stream from 0.

This reduces the time that it takes for the blob pruner to run from 60 secs to ~1-2 secs on rocksdb.
{"@timestamp":"2025-01-28T08:52:00,212","level":"DEBUG","thread":"storagePrunerAsyncRunner-async-1","class":"BlobSidecarPruner","message":"Pruning blobs up to slot 3381555, limit 12","throwable":""} {"@timestamp":"2025-01-28T08:52:01,736","level":"INFO","thread":"validator-async-1","class":"teku-validator-log","message":"Validator *** Published attestation Count: 1, Slot: 3512660, Root: f70a2b57992861ea63f576d7d0cc5bf891d2d2c475cac54ae42e61d0b8ca71d5","throwable":""} {"@timestamp":"2025-01-28T08:52:01,991","level":"DEBUG","thread":"storagePrunerAsyncRunner-async-1","class":"KvStoreDatabase","message":"Pruned 12 BlobSidecars","throwable":""} {"@timestamp":"2025-01-28T08:52:01,991","level":"DEBUG","thread":"storagePrunerAsyncRunner-async-1","class":"BlobSidecarPruner","message":"Blobs pruning finished in 1779 ms. Limit reached: true","throwable":""}

Fixed Issue(s)

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.

Signed-off-by: Gabriel Fukushima <[email protected]>
…ng streaming from 0 all the time.

Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
…arliestFinalizedBlock

Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
…g creating streaming from 0 all the time."

This reverts commit 0ca062a

Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
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.

generally LGTM I just want to check on a pre-existing thing that has been recently introduced (maybe a quick chat on it?)

@@ -436,7 +449,7 @@ private UInt64 pruneToBlock(
}

try (final Stream<SignedBeaconBlock> stream =
dao.streamFinalizedBlocks(UInt64.ZERO, checkpointInitialSlot)) {
dao.streamFinalizedBlocks(earliestFinalizedBlockSlot, checkpointInitialSlot)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these two rounds of streamFinalizedBlocks are a bit of a mystery to me.
why we need the second? and if we really need it why we cant simply stream keys (we just need slots here and the slot in the key is always block.getSlot()).

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 haven't looked into this optimization tbh, sound fair but I think I ran into issues when I first implemented this with 1 stream only

Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
# Conflicts:
#	storage/src/main/java/tech/pegasys/teku/storage/server/kvstore/KvStoreDatabase.java
Signed-off-by: Gabriel Fukushima <[email protected]>
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. let's address my comment on another PR

@tbenr tbenr merged commit 866725f into Consensys:master Jan 29, 2025
16 checks passed
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