[wip] Goodbye manual rocksdb compaction#27529
[wip] Goodbye manual rocksdb compaction#27529ryoqun wants to merge 5 commits intosolana-labs:masterfrom
Conversation
|
@yhchiang-sol hey, could you take over this pr as a new blockstore expert, if possible? this pr is very draft. i just removed the relevant code and saw remaning tasks:
cc: @CriesofCarrots (as original primary index implmenter), @carllin (as past reviewer of relevant merged pr: #16697), @steviez (recently, i sensed you're nicely tagged with @yhchiang-sol as blockstore reviewer ) |
| [default: all validators]") | ||
| ) | ||
| .arg( | ||
| Arg::with_name("no_rocksdb_compaction") |
There was a problem hiding this comment.
i think it's time to remove these flags finally, even at the cost of some command line compatility churn.
so, we need to announce this and following some flags to be stripped from all scripts well in advance. After all, these flags is no-op as of now. (cc: @willhickey; this pr won't be merged soon. just very early headsup)
|
|
||
| let private_rpc = matches.is_present("private_rpc"); | ||
| let do_port_check = !matches.is_present("no_port_check"); | ||
| let no_rocksdb_compaction = true; |
There was a problem hiding this comment.
@yhchiang-sol i think you can tell all the deleted code here is indded unused because of this hard-coded flag.. ;) this was changed at #16697
xref: https://github.com/solana-labs/solana/pull/27529/files#r960164605
| if self.no_compaction { | ||
| info!("compact_storage: compaction disabled"); | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
| /// A faster approximation of `Exact` where the purge process only takes | ||
| /// care of the primary index and does not update the associated entries. | ||
| PrimaryIndex, |
There was a problem hiding this comment.
i think this is another good starting point to understand this pr.
|
@ryoqun: FYI, so I am also drafting #27567: it introduces a new ShredStorageType RocksSlotTtl that has similar/same performance as FIFO but also honors our slot concept and is compatible with the existing level compaction. I am still baking the PR and will soon run some experiments. If the experimental sound, then it's probably better than both FIFO and the default Level. |
Aghh, that doesn't work because DeleteFilesInRange is currently not supported in L0. Such a pity :'(. |
github-actions
Bot
added
the
stale
2 participants
Problem
manual compaction code and its accompanying
PrimaryIndexpurge mechanism can be decommissioned, now that we've transitioned to the compaction filter, which is initially introduced at #16697 (1+ year ago)also, these unused code is still maintained, indicating confusion for newer team members due to my laziness: ref #27201
Ideally, i'd like this to be merged before the shiny new fifo compaction come in across all the live clusters to avoid three separate impls.
Summary of Changes
Remove it