Skip to content

Add rocksdb_lru_cache_options_set_high_pri_pool_ratio() to the C API - #15573

Open
niklasf wants to merge 1 commit into
meta:mainfrom
niklasf:high-pri-pool-ratio-c-api
Open

niklasf wants to merge 1 commit into
meta:mainfrom
niklasf:high-pri-pool-ratio-c-api

Conversation

@niklasf

@niklasf niklasf commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@meta-cla meta-cla Bot added the CLA Signed label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

✅ clang-tidy: No findings on changed lines

Completed in 74.9s.

@github-actions

github-actions Bot commented Nov 3, 2026

Copy link
Copy Markdown

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 439a876


Summary

Clean, minimal change that correctly exposes an existing LRUCacheOptions field through the C API. The implementation follows existing patterns and includes a test and release note.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Missing low_pri_pool_ratio setter for API completeness — include/rocksdb/c.h
  • Issue: LRUCacheOptions has two related pool ratio fields: high_pri_pool_ratio and low_pri_pool_ratio. This PR exposes only high_pri_pool_ratio. The C API is now inconsistent — it exposes one half of a paired configuration without the other. Users who want to set high_pri_pool_ratio via the C API will often also want to set low_pri_pool_ratio (the two are validated together: their sum must not exceed 1.0).
  • Root cause: Incomplete API surface coverage.
  • Suggested fix: Add rocksdb_lru_cache_options_set_low_pri_pool_ratio() in the same PR, or note it as a follow-up.
M2. No input validation in the C API setter — db/c.cc:7825
  • Issue: The C++ LRUCache constructor validates that high_pri_pool_ratio is in [0.0, 1.0] and that high_pri_pool_ratio + low_pri_pool_ratio <= 1.0 (see cache/lru_cache.cc:704-713), returning an invalid argument status on failure. However, the C API setter silently accepts any double value. If a user passes a negative value or a value > 1.0, they won't discover the error until cache creation time. This is consistent with the existing C API setters (which also don't validate), so it's not a regression — but worth noting that validation is deferred to cache construction.
  • Root cause: The existing C API pattern for LRU cache options setters doesn't do eager validation. This is acceptable since validation happens at MakeSharedCache() time.
  • Suggested fix: No change required for consistency, but consider documenting the valid range in the c.h header comment.

🟢 LOW / NIT

L1. Test could verify high-pri pool ratio takes effect — db/c_test.c:5256
  • Issue: The test sets high_pri_pool_ratio to 0.2 and verifies the cache is created with the correct capacity, but doesn't verify that the high-priority pool ratio actually took effect. While this is consistent with the test style of the existing cache_options test block above it, a stronger test could insert entries with different priorities and verify eviction behavior, or use an internal getter if available.
  • Root cause: Minimal test coverage, though consistent with existing patterns.
  • Suggested fix: Acceptable as-is given the pattern of existing tests. The C++ side already has extensive tests for high_pri_pool_ratio behavior.
L2. Consider adding strict_capacity_limit and use_adaptive_mutex setters — include/rocksdb/c.h
  • Issue: Beyond low_pri_pool_ratio, the C API is also missing setters for strict_capacity_limit and use_adaptive_mutex. These are less critical but noted for completeness.
  • Suggested fix: Future follow-up work, not a blocker for this PR.

Cross-Component Analysis

Context Affected? Notes
DB Mutex No This is a configuration setter, not a runtime operation
Thread safety N/A Sets a field on a config struct before cache construction
Serialization N/A No format changes
Behavioral compatibility No Purely additive new API

Positive Observations

  • The implementation correctly mirrors the existing setter pattern (set_capacity, set_num_shard_bits, set_memory_allocator).
  • Both db/c.cc and tools/c_api_gen/c_base.cc / c_base.h are kept in sync.
  • Test and release note are included.
  • The release note is appropriately concise.

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

  • Claude may miss context from files not in the diff
  • Large PRs may be truncated
  • Always apply human judgment to AI suggestions

Commands:

  • /claude-review [context] — Request a code review
  • /claude-query <question> — Ask about the PR or codebase

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant