Add a bottommost option to compact() so existing data can be re-encoded - #740
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for a bottommost option in the compact and compactSync methods, allowing users to force compaction of the bottommost level in RocksDB to re-encode existing data under a new compression codec. The changes span the C++ bindings, TypeScript wrappers, documentation, and tests. The feedback highlights a compatibility issue in the newly added tests where entry.parentPath is used, which will fail on Node.js 18; a fallback to entry.path is recommended to maintain compatibility.
📊 Benchmark Resultsget-sync.bench.tsgetSync() > random keys - small key size (100 records)
getSync() > sequential keys - small key size (100 records)
ranges.bench.tsgetRange() > small range (100 records, 50 range)
realistic-load.bench.tsRealistic write load with workers > write variable records with transaction log
transaction-log.bench.tsTransaction log > read 100 iterators while write log with 100 byte records
Transaction log > read one entry from random position from log with 1000 100 byte records
worker-put-sync.bench.tsputSync() > random keys - small key size (100 records, 10 workers)
worker-transaction-log.bench.tsTransaction log with workers > write log with 100 byte records
Results from commit 59666ea |
compactRange passed a default-constructed CompactRangeOptions, whose
bottommost_level_compaction is kIfHaveCompactionFilter. With no compaction filter
installed, that skips the bottommost level — where the bulk of the data sits — so
compact() could not rewrite it.
That matters because a column family's compression governs newly written files only.
Existing SST and blob files keep their original codec until something rewrites them,
and nothing available could: changing the codec on a database that already held data
left that data encoded as before, indefinitely. Measured on 60k compressible records
written uncompressed and reopened as zstd: 34.8M, unchanged by compact(), and 2.0M
after compact({ bottommost: true }).
Opt-in rather than the default because it rewrites the entire range regardless of
whether RocksDB judges it worthwhile, so it costs as much as the data is large.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5fef7ec to
6f766b5
Compare
…e too bottommost:true only rewrote SST files (kForce). Blob GC's default age cutoff reclaims just the oldest fraction of blob files, so values at or above the 2048-byte blob threshold stayed on the old codec after a "full" migration. Switch to kForceOptimized (still avoids double-compacting bottommost files from the same manual compaction) and force blob GC across the full age range (kForce, age cutoff 1.0) so bottommost compaction re-encodes blobs as well as SSTs. Adds a regression covering the blob path (>2048-byte values) alongside the existing SST-only test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… doc accuracy
- test/compaction.test.ts: write the blob-backed regression in three
separate flushes (three blob-file generations) instead of one, and
assert values read back correctly from every generation after the
forced bottommost compaction — the prior version only checked
aggregate .blob byte counts, which a partial re-encode or corrupting
rewrite could still pass.
- Fix a comment that claimed re-running compactSync({bottommost:true})
is a "no-op" — kForceOptimized avoids double-compacting bottommost
files from the same manual compaction, not across separate calls.
- README.md: "an ordinary compact() will not [rewrite existing data]"
overstated it — plain compact() does rewrite non-bottommost levels,
it just skips the bottommost level where most data sits. Also note
the forced rewrite is per column family.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The blob regression only called the async compact() entry point, so a wiring bug that dropped the flag on the sync N-API path would still pass. Restrict the async call to the first blob generation's key range and let compactSync recode the rest, then verify values from every generation survive intact — proving both entry points force blob GC on their own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
README documents Node 18+ support, but entry.parentPath (used by the sst/blob byte-counting helper) was only added in Node 20.11/21.5 — on Node 18 it's undefined, the helper fell back straight to the top-level dir, and stat() on a nested file would ENOENT. Fall back through the older `path` property first, as gemini-code-assist flagged on this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Reviewed Confirmed the native path: — |
Register the codec migration regressions with it.skipIf so builds without zstd expose the missing coverage as skipped instead of silently passing after an early return. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Use the exclusive end key immediately after the first flushed batch so the async range compaction matches the test comment and covers all 700 records before compactSync handles the remainder. Co-Authored-By: GPT-5 Codex <noreply@openai.com>
compactRangepassed a default-constructedCompactRangeOptions, whosebottommost_level_compactioniskIfHaveCompactionFilter. With no compaction filter installed that skips the bottommost level — where the bulk of the data sits — socompact()could not rewrite it.That matters because a column family's
compressiongoverns newly written files only. Existing SST and blob files keep the codec they were written with until something rewrites them, and nothing available could. Changing the codec on a database that already held data left that data encoded as before, indefinitely.Measured on 60k compressible records written uncompressed and reopened as zstd:
Opt-in rather than the default because it rewrites the entire range regardless of whether RocksDB judges it worthwhile, so it costs as much as the data is large — it should be a deliberate operator action, not something an upgrade or a routine compaction does implicitly.
Why this is needed
harper#2044 makes Harper honorstorage.compressionon databases upgraded from 5.1, which had been silently ignored — an existing column family inherits its persisted codec on reopen rather than adopting the build default, so those instances were writing uncompressed indefinitely. That fix compresses new writes, which is the right default. This gives operators the separate, explicit action to convert the data already on disk; without it there is no path from an existing uncompressed database to a compressed one short of a dump and reload.Changes
db_descriptor.cpp/.h—compactRangetakes abottommostflag and setsBottommostLevelCompaction::kForcedatabase.cpp/.h— the flag is read from an added trailing argument on bothCompactandCompactSync, and carried onAsyncCompactStatestore.ts—CompactOptions.bottommost?: boolean, forwarded to both native callsload-binding.ts— native signatures updatedcompressionon an existing family inherits its codec rather than applying the defaultTesting
test/compaction.test.tsgains a case that writes uncompressed, reopens under zstd, asserts a plaincompact()leaves the SST bytes exactly unchanged, then asserts{ bottommost: true }drops them below half — through bothcompact()andcompactSync(). Skipped when the native build lacks zstd. 39 tests pass across the compaction and compression suites.Verified against a RocksDB prebuild carrying the compression codecs (11.1.2); the repo's currently vendored prebuild has only none/zlib, so the new test self-skips there.
Generated by Claude Opus 5.
🤖 Generated with Claude Code