This repository was archived by the owner on Jan 22, 2025. It is now read-only.
Remove Blockstore manual compaction code#28409
Merged
steviez merged 1 commit intosolana-labs:masterfrom Oct 28, 2022
Merged
Conversation
steviez
commented
Oct 15, 2022
steviez
commented
Oct 15, 2022
steviez
commented
Oct 15, 2022
Contributor
yhchiang-sol
left a comment
yhchiang-sol
left a comment
There was a problem hiding this comment.
Thanks for working on this! The removal of the manual compaction code looks good to me.
The only real comment is the ending check of the ledger_cleanup_test.
steviez
force-pushed
the
remove_compaction
branch
from
10fe2f8 to
6997820
Compare
steviez
commented
Oct 27, 2022
Contributor
yhchiang-sol
left a comment
yhchiang-sol
left a comment
There was a problem hiding this comment.
Great we share thoughts in common. Here're my thoughts about this:
- Move forward with this PR. Improvements/changes for the test go to a separate PR.
- Remove the ending manual compaction check in the test in this PR.
The manual Blockstore compaction that was being initiated from LedgerCleanupService has been disabled for quite some time in favor of several optimizations. Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
steviez
force-pushed
the
remove_compaction
branch
from
6997820 to
fbc75c6
Compare
Contributor
Author
|
I think Ryo is currently preoccupied with some other more important stuff, but given that Ryo was the one who initially made the PR, pushing this in and will be happy to do address any potential issues in followup PRs. There will probably be a subsequent PR at some point to rip out the purge stuff. We should 100% wait for ship-its from Tyera and/or Ryo on that one |
gnapoli23
pushed a commit
to gnapoli23/solana
that referenced
this pull request
Dec 16, 2022
The manual Blockstore compaction that was being initiated from LedgerCleanupService has been disabled for quite some time in favor of several optimizations. Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The manual Blockstore compaction that was being initiated from LedgerCleanupService has been disabled for quite some time in favor of several optimizations.
Summary of Changes
Rip out the dead code (and eliminate one thread)
I was poking around LedgerCleanupService following an idea I discussed with Yueh-Hsuan, and then was reminded of the dead code in there. So, I decided to pick up some of the cleanup from #27529. However, that PR modified compaction and purge routines. I thought it'd be more manageable to handle one item per PR, so here is just the compaction stuff. The compaction removal is more straightforward than the purge change as well, so this should be a less complex review.