fullhistory: scope the txhash tuning knobs to the txhash CF (#838)#849
Conversation
chowbao
force-pushed
the
chore/fullhistory-hardening-840
branch
3 times, most recently
from
8a10b76 to
604a021
Compare
Grow CFOptions with the memtable, compaction, and bloom knobs that were DB-wide fields on Tuning, and route the bloom filter through the per-CF override. Tuning now carries only the genuinely DB-wide resources shared across every CF of one store: block cache, WAL cap, background jobs, and open-file cap. This lets a facade scope a workload's tuning to the CF that has that workload instead of leaking it onto every CF in the shared per-chunk DB.
Split txhash's config into the DB-wide Tuning it always set (block cache, WAL cap, background jobs, open files) and a new CFOptions carrying its per-CF workload knobs (bloom, write buffers, compaction triggers). The hotchunk opener merges the per-CF options from every facade so those knobs land on the txhash CF alone. Restores each store's pre-unification standalone tuning: the ledgers CF runs with no bloom filter (it is never probed for missing keys) and default memtables/compaction; the events CFs keep their ZSTD and block-size overrides; the txhash CF keeps its full calibration. The DB-wide resources stay shared, sized to what the three standalone instances used together (only txhash set them).
chowbao
force-pushed
the
chore/fullhistory-percf-tuning-838
branch
from
df54550 to
6a6a32e
Compare
| // CFOptions returns the calibration the hotchunk opener installs on the | ||
| // txhash CF alone. The workload is write-once / point-lookup; only it | ||
| // earned this tuning — the other CFs ride on RocksDB defaults. | ||
| func CFOptions() map[string]rocksdb.CFOptions { |
There was a problem hiding this comment.
Are there any options worth removing from here or tuning.go?
Note actual values used for RPC v2 can be updated/corrected when full v2 benchmarking is run
There was a problem hiding this comment.
Two removals landed since: MaxBytesForLevelBaseMB (a documented no-op under DisableAutoCompactions) was dropped from CFOptions end-to-end in 4e0c912, and txhash.Tuning() itself is gone in 5b018a0 (the DB-wide knobs moved to hotchunk.dbTuning(), per the review). The remaining fields are all live: each is consumed by applyCFOverride/applyDBTuning and set by at least one facade. Value re-calibration deferred to v2 benchmarking as noted.
|
@codex[agent] review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Co-authored-by: chowbao <20801401+chowbao@users.noreply.github.com>
tamirms
left a comment
tamirms
left a comment
There was a problem hiding this comment.
The per-CF split looks right. One thing I would like addressed in this PR before it merges.
hotchunk.config() now assembles the whole rocksdb.Config, pulling the per-CF options from each facade that owns those CFs (eventstore.CFOptions() and txhash.CFOptions()). But it still sources the DB-wide half from a single tenant:
Tuning: txhash.Tuning(),The Tuning fields (block cache, WAL cap, background jobs, open files) describe the shared multi-CF DB that hotchunk owns, not the txhash facade. The catalog, which is the other RocksDB database here, already configures its own DB inline rather than borrowing from a store:
store, err := rocksdb.New(rocksdb.Config{Path: path, Logger: logger})The tell is change-locality: if getEvents later wants a larger block cache, you would be editing txhash.Tuning() to size a cache for an events-driven reason. Some of these values are already not txhash-specific anymore, since the background jobs now drive the ledger and events compaction this PR re-enables, and the cache serves every CF.
This is the PR that closes #838, whose whole point is to stop txhash from configuring things that are not its CF, and the shared DB is the largest such thing. As it stands the change fixes the per-CF half of that coupling but leaves the DB-wide half in place, so it reads as half-done against its own goal. I would define the DB-wide Tuning in hotchunk (in or beside config(), with a comment crediting the txhash calibration as its origin), keep txhash.CFOptions(), and drop txhash.Tuning(). Then the rule is uniform: each facade configures its own CFs, and the DB owner configures the DB, matching the catalog.
|
Addressed in 5b018a0: the DB-wide |
3 participants
Closes #838.
The shared hot-chunk config applied txhash's
Tuning(12-bit bloom, 64 MB write buffers, no-compaction triggers) to the ledgers and events CFs too. Per the issue's sharpened scope, this is a mechanical port of each store's pre-unification standalone tuning (verified againstc445557d^).Commit 1 moves the memtable/compaction/bloom knobs from DB-wide
Tuninginto per-CFrocksdb.CFOptions(each still zero-means-default).Tuningkeeps only the genuinely DB-wide resources: block cache, WAL cap, background jobs, open-file cap. The bloom filter is still constructed fresh per CF, now only for CFs that opt in.Commit 2 scopes the values:
The DB-wide values (512 MB cache, 1 GB WAL, 8 jobs, 10k files) are now defined in
hotchunk.dbTuning()— the DB owner configures the DB, each facade configures only its own CFs;txhash.Tuning()is gone. The values keep the standalone txhash calibration, the only pre-unification instance that set them.Tests:
TestConfig_PerCFOptionRoutingandTestConfig_DBWideTuningStaysSharedpin the routing and shared values at the config seam. Fullfullhistory-shortsuite and-raceonrocksdb/hotchunkpass.