-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Reduce the amount of IO that LedgerCleanupService performs #29239
Conversation
0e83747
to
7858c35
Compare
7858c35
to
3639d23
Compare
3639d23
to
4a4d952
Compare
f9be872
to
3cb3baf
Compare
3cb3baf
to
aa133dd
Compare
83b7d6f
to
35533ef
Compare
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.
The PR looks good! Only minor comments.
Btw, do you happen to have updated numbers from your experiments? Or the previous numbers are already from the updated PR?
Was planning on re-running with rebased on latest; going to utilize the nodes I had been using for testing the SlotMeta one. I consider getting another datapoint as a hard requirement before shipping. It'd be nice to get a graph with I/O isolated to the blockstore (like we did on the GCP nodes when we put everything on separate drives), but with the dev servers having everything on one drive, it is harder to get a clean measurement. I don't consider this a hard requirement for shipping this PR tho; from inspection, it is very obvious that we are saving IO by not reading the SlotMeta column |
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.
The PR looks good! Thanks for adding tests! (esp. the test is tricky to write since there're things that are still in mem-tables, manual flush in tests is a good workaround.)
I don't consider this a hard requirement for shipping this PR tho; from inspection, it is very obvious that we are saving IO by not reading the SlotMeta column
Definitely not a hard requirement, but it would be great if we can explicitly say how much this PR improves things.
Agreed. We do know how large the |
35533ef
to
50bb1ac
Compare
Pull request has been modified.
Currently, the cleanup service counts the number of shreds in the database by iterating the entire SlotMeta column and reading the number of received shreds for each slot. This gives us a fairly accurate count at the expense of performing a good amount of IO. Instead of counting the individual slots, use the live_files() rust-rocksdb entrypoint that we expose in Blockstore. This API allows us to get the number of entries (shreds) in the data shred column family by reading file metadata. This is much more efficient from IO perspective.
50bb1ac
to
2dfad7a
Compare
Problem
Currently, the cleanup service counts the number of shreds in the database by iterating the entire SlotMeta column and reading the number of received shreds for each slot. This gives us a fairly accurate count at the expense of performing a good amount of IO.
Summary of Changes
Instead of counting the individual slots, use the live_files() rust-rocksdb entrypoint that we expose in Blockstore. This API allows us to get the number of entries (shreds) in the data shred column family by reading file metadata. This is much more efficient from IO perspective.
Fixes #28403