Skip to content

cherry-pick: Make RocksDB memory config configurable (#780) → v5.0 - #1133

Merged
kriszyp merged 3 commits into
v5.0from
cherry-pick/v5.0/pr-780
Jun 5, 2026
Merged

cherry-pick: Make RocksDB memory config configurable (#780) → v5.0#1133
kriszyp merged 3 commits into
v5.0from
cherry-pick/v5.0/pr-780

Conversation

@kriszyp

@kriszyp kriszyp commented Jun 4, 2026

Copy link
Copy Markdown
Member

Backport of #780 to v5.0.

Cherry-picks the four commits that make up PR #780 onto the v5.0 release branch, exposing four new storage.rocks.* config params:

  • storage.rocks.blockCacheSize — bytes for the shared block cache (defaults to 25% of constrained memory)
  • storage.rocks.writeBufferManagerSize — bytes the process-wide WriteBufferManager caps total memtable memory at (0 = disabled, the default)
  • storage.rocks.writeBufferManagerCostToCache — charge memtable bytes against the block cache for unified accounting
  • storage.rocks.writeBufferManagerAllowStall — stall writes once the WBM cap is hit vs. allow brief overshoot

Why backport: lets operators bound the RocksDB memory footprint without rebuilding. On shared Fabric hosts the per-CF maintain window can hold ~1.8 GB of anonymous memory by default; this gives a knob to cap it via env vars / YAML.

Conflicts during cherry-pick (resolved):

  • resources/databases.ts: incoming change collided with the read-only-mode (isReadOnlyMode()) block from main. v5.0 doesn't have read-only mode, so I kept the new WBM config block and dropped the read-only lines.
  • utility/hdbTerms.ts: incoming STORAGE_READONLY entry alongside the four new ROCKS params. Dropped STORAGE_READONLY (not on v5.0), kept the four ROCKS params.

The cumulative diff vs. v5.0 matches PR #780's effect on main (minus the unrelated read-only-mode lines). Net change: 2 files, +36/−3.

Forward-compatibility: WBM options are no-ops until @harperfast/rocksdb-js ships WBM support and harper-pro bumps its dep. blockCacheSize works today. The conditional-spread on RocksDatabase.config() means unset values never reach older bindings.

Generated by Claude (Opus 4.7).

kriszyp and others added 3 commits June 4, 2026 08:58
Adds three new config params for the process-wide RocksDB
WriteBufferManager exposed by @harperfast/rocksdb-js#584:

- storage.rocks.writeBufferManagerSize — bytes the WBM caps across
  all DBs in the process (0 disables). The structural cap on total
  memtable + maintain-window memory.
- storage.rocks.writeBufferManagerCostToCache — when true, memtable
  charges show up in block-cache-usage as pinned entries (single
  observability metric for read cache + memtable footprint).
- storage.rocks.writeBufferManagerAllowStall — when true, writes
  stall once the cap is reached instead of allowing memtables to
  briefly exceed it.

Values are read inside openRocksDatabase() and passed via spread,
so unset values don't surface unknown options to older bindings of
rocksdb-js where the WBM isn't yet wired up. This makes the change
forward-compatible: it's a no-op until the env var is set AND the
underlying binding supports it.

Co-Authored-By: Claude Sonnet 4.7 <noreply@anthropic.com>
(cherry picked from commit f24bae0)
envGet may return raw strings from process.env (configUtils doesn't
cast every code path), so RocksDatabase.config previously could
receive "12345" instead of 12345 for the size params and "false"
(truthy in JS) instead of false for the booleans.

- Numeric params (blockCacheSize, writeBufferManagerSize): wrap with
  Number(). Falsy results (NaN, 0) fall through the > 0 guard.
- Boolean params (writeBufferManagerCostToCache, allowStall): inline
  toBool() that recognizes booleans, 'true'/'false' strings, and
  generic truthy/falsy fallback. Returns undefined when unset so the
  spread skips the property.

Per PR review from @cb1kenobi (PR #780, line 169).

Co-Authored-By: Claude Sonnet 4.7 <noreply@anthropic.com>
(cherry picked from commit bc725ea)
Reverts 064501b's Number()/toBool() coercion in favor of strict
type checks. Config values flow through configUtils.castConfigValue
which produces proper numbers/booleans from YAML, env vars, and CLI
args — anything else arriving here is misconfiguration that should
fall through to the default rather than be silently rescued.

- blockCacheSize / writeBufferManagerSize: only honored when
  `typeof === 'number' && value > 0`. Non-numbers (including
  unparseable strings) fall back to the 25% default for the cache
  and disable the WBM.
- writeBufferManagerCostToCache / allowStall: only honored when
  `typeof === 'boolean'`. The 'false' string is no longer accepted
  — it's a YAML-quoting mistake the operator should fix.

Per feedback on PR #780 review thread.

Co-Authored-By: Claude Sonnet 4.7 <noreply@anthropic.com>
(cherry picked from commit 46109e6)
@kriszyp
kriszyp requested a review from a team as a code owner June 4, 2026 14:59
@kriszyp kriszyp added the patch label Jun 4, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces lazy configuration of RocksDB memory parameters (such as block cache size and write buffer manager settings) based on environment variables, and registers these new configuration keys in hdbTerms.ts. Feedback on the changes highlights that calling RocksDatabase.config on every invocation of openRocksDatabase is highly inefficient and potentially dangerous, as it runs repeatedly for every table or index and could recreate shared caches or cause memory leaks. It is recommended to guard this initialization so it only runs once and wrap it in a try-catch block to prevent any unexpected configuration errors from blocking the critical database startup path.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread resources/databases.ts
Comment on lines +127 to +146
const configuredBlockCacheSize = envGet(CONFIG_PARAMS.STORAGE_ROCKS_BLOCKCACHESIZE);
const blockCacheSize =
typeof configuredBlockCacheSize === 'number' && configuredBlockCacheSize > 0
? configuredBlockCacheSize
: Math.min(process.constrainedMemory?.() ?? Infinity, totalmem()) * 0.25;
const writeBufferManagerSize = envGet(CONFIG_PARAMS.STORAGE_ROCKS_WRITEBUFFERMANAGERSIZE);
const writeBufferManagerCostToCache = envGet(CONFIG_PARAMS.STORAGE_ROCKS_WRITEBUFFERMANAGERCOSTTOCACHE);
const writeBufferManagerAllowStall = envGet(CONFIG_PARAMS.STORAGE_ROCKS_WRITEBUFFERMANAGERALLOWSTALL);
RocksDatabase.config({
blockCacheSize,
...(typeof writeBufferManagerSize === 'number' && writeBufferManagerSize > 0
? { writeBufferManagerSize }
: {}),
...(typeof writeBufferManagerCostToCache === 'boolean'
? { writeBufferManagerCostToCache }
: {}),
...(typeof writeBufferManagerAllowStall === 'boolean'
? { writeBufferManagerAllowStall }
: {}),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Calling RocksDatabase.config on every invocation of openRocksDatabase is highly inefficient and potentially dangerous. Since openRocksDatabase is called for every table, index, and internal DBI, this repeatedly re-reads environment variables and re-applies the global RocksDB configuration. Depending on the underlying @harperfast/rocksdb-js implementation, repeatedly calling config could recreate the shared block cache and write buffer manager, leading to memory leaks, multiple independent caches, or invalidation of existing cached blocks.

Since these process-level configurations do not change after startup, we should ensure RocksDatabase.config is only called once. We can guard this initialization using a property on the openRocksDatabase function itself. Additionally, because this is a non-critical optimization executed during a critical startup path, we should wrap it in a try-catch block to ensure that any unexpected failure does not block the critical database opening process.

if (!(openRocksDatabase as any).configured) {
	(openRocksDatabase as any).configured = true;
	try {
		const configuredBlockCacheSize = envGet(CONFIG_PARAMS.STORAGE_ROCKS_BLOCKCACHESIZE);
		const blockCacheSize =
			typeof configuredBlockCacheSize === 'number' && configuredBlockCacheSize > 0
				? configuredBlockCacheSize
				: Math.min(process.constrainedMemory?.() ?? Infinity, totalmem()) * 0.25;
		const writeBufferManagerSize = envGet(CONFIG_PARAMS.STORAGE_ROCKS_WRITEBUFFERMANAGERSIZE);
		const writeBufferManagerCostToCache = envGet(CONFIG_PARAMS.STORAGE_ROCKS_WRITEBUFFERMANAGERCOSTTOCACHE);
		const writeBufferManagerAllowStall = envGet(CONFIG_PARAMS.STORAGE_ROCKS_WRITEBUFFERMANAGERALLOWSTALL);
		RocksDatabase.config({
			blockCacheSize,
			...(typeof writeBufferManagerSize === 'number' && writeBufferManagerSize > 0
				? { writeBufferManagerSize }
				: {}),
			...(typeof writeBufferManagerCostToCache === 'boolean'
				? { writeBufferManagerCostToCache }
				: {}),
			...(typeof writeBufferManagerAllowStall === 'boolean'
				? { writeBufferManagerAllowStall }
				: {}),
		});
	} catch (error) {
		logger.error('Failed to apply RocksDB configuration', error);
	}
}
References
  1. Ensure that non-critical cleanup or optimization operations executed during critical startup or recovery paths are wrapped in try-catch blocks so that their failure does not block the critical process.

@kriszyp
kriszyp merged commit e114573 into v5.0 Jun 5, 2026
20 of 23 checks passed
@kriszyp
kriszyp deleted the cherry-pick/v5.0/pr-780 branch June 5, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants