Skip to content

Fix spelling of memtable_verify_per_key_checksum_on_seek#14811

Closed
xingbowang wants to merge 3 commits into
facebook:mainfrom
xingbowang:2026_06_01_fix_memtable_verify_per_key_checksum_on_seek
Closed

Fix spelling of memtable_verify_per_key_checksum_on_seek#14811
xingbowang wants to merge 3 commits into
facebook:mainfrom
xingbowang:2026_06_01_fix_memtable_verify_per_key_checksum_on_seek

Conversation

@xingbowang

@xingbowang xingbowang commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Rename the misspelled memtable_veirfy_per_key_checksum_on_seek option and related flags/config keys to memtable_verify_per_key_checksum_on_seek.
  • Update memtable option plumbing, options serialization/logging, db_bench/db_stress/crash-test flags, tests, and the option-addition guide to use the corrected name.
  • Keep the checksum-on-seek behavior unchanged.

Test Plan

CI

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

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

⚠️ clang-tidy: 9 warning(s) on changed lines

Completed in 1661.7s.

Summary by check

Check Count
cert-err58-cpp 4
clang-analyzer-optin.core.EnumCastOutOfRange 4
cppcoreguidelines-special-member-functions 1
Total 9

Details

db/blob/db_blob_direct_write_test.cc (1 warning(s))
db/blob/db_blob_direct_write_test.cc:127:7: warning: class 'ActiveBlobVisibilityWritableFile' defines a non-default destructor but does not define a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
include/rocksdb/file_system.h (4 warning(s))
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
include/rocksdb/file_system.h:195:10: warning: The value '3' provided to the cast expression is not in the valid range of values for 'FileOpenContract' [clang-analyzer-optin.core.EnumCastOutOfRange]
tools/ldb_cmd.cc (4 warning(s))
tools/ldb_cmd.cc:5469:19: warning: initialization of 'kInputFile' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
tools/ldb_cmd.cc:5470:19: warning: initialization of 'kResultFile' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
tools/ldb_cmd.cc:5537:51: warning: initialization of 'ARG_JOB_DIR' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
tools/ldb_cmd.cc:5590:50: warning: initialization of 'ARG_JOB_DIR' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
@meta-codesync

meta-codesync Bot commented Jun 3, 2026

Copy link
Copy Markdown

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D107382276.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit ef6bff9


Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit ef6bff9


Summary

Mechanical spelling fix renaming memtable_veirfy_per_key_checksum_on_seek to memtable_verify_per_key_checksum_on_seek across 17 files. The rename is consistent and complete across all code, tests, tools, and documentation. However, there is a high-severity backwards compatibility issue: the serialization key string in the options type info map is renamed without preserving the old spelling as a deprecated alias, which will break loading of OPTIONS files written by RocksDB 10.7.0+.

High-severity findings (1):

  • [options/cf_options.cc:705] Renaming the serialization key string breaks loading of existing OPTIONS files that contain the old (misspelled) key name.
Full review (click to expand)

Findings

🔴 HIGH

H1. OPTIONS file backwards compatibility broken -- options/cf_options.cc:705
  • Issue: The string key "memtable_veirfy_per_key_checksum_on_seek" in the cf_mutable_options_type_info map is the serialization key written to OPTIONS files. This option was released in RocksDB 10.7.0 (Sept 2025) with the misspelled name. Any existing database's OPTIONS file will contain memtable_veirfy_per_key_checksum_on_seek=.... After this rename, loading those OPTIONS files will fail with Status::NotFound (when ignore_unknown_options=false, the default for same-version loading) or silently lose the setting (when ignore_unknown_options=true).
  • Root cause: The serialization key string is the externally-visible, persisted name. Renaming it is an API/format breaking change.
  • Suggested fix: Keep the old misspelled key as a deprecated alias (using the established pattern at cf_options.cc:35-38), and add the new correctly-spelled key as the primary:
    // Old misspelled key kept for backwards compatibility with OPTIONS files
    {"memtable_veirfy_per_key_checksum_on_seek",
     {offsetof(struct MutableCFOptions,
               memtable_verify_per_key_checksum_on_seek),
      OptionType::kBoolean, OptionVerificationType::kDeprecated,
      OptionTypeFlags::kMutable}},
    // Corrected spelling (primary)
    {"memtable_verify_per_key_checksum_on_seek",
     {offsetof(struct MutableCFOptions,
               memtable_verify_per_key_checksum_on_seek),
      OptionType::kBoolean, OptionVerificationType::kNormal,
      OptionTypeFlags::kMutable}},
    This pattern is used extensively in cf_options.cc (see lines 291, 398, 401, 412, etc.) and is documented in the file header comment (lines 35-38).

🟡 MEDIUM

No medium-severity findings.

🟢 LOW / NIT

L1. options_settable_test.cc should accept both spellings -- options/options_settable_test.cc:718
  • Issue: If the deprecated alias is added per H1, consider adding a test that verifies the old spelling is still accepted (round-trip test for deprecated option loading).
L2. HISTORY.md already uses the corrected spelling -- HISTORY.md:156
  • Issue: The release note in HISTORY.md for 10.7.0 already says memtable_verify_per_key_checksum_on_seek (correct spelling), which is inconsistent with the actual option name in that release. Pre-existing issue, not introduced by this PR.
L3. Missing release note for the rename
  • Issue: No unreleased_history/ entry documenting this rename. Users with scripts or configs referencing the old flag name need to know about the change.
  • Suggested fix: Add a release note noting the rename and (if H1's fix is adopted) that the old spelling is still accepted for OPTIONS file loading.

Cross-Component Analysis

Context Impact Assessment
OPTIONS file persistence Old OPTIONS files become unloadable BREAKING -- requires deprecated alias
db_bench CLI flags Users with scripts using old flag name will get errors Breaking but acceptable (CLI flags are not persisted)
db_crashtest.py Only affects test tooling Safe
SetOptions() dynamic config Users calling SetOptions({"memtable_veirfy_...", "true"}) will get errors Breaking but unlikely usage

Positive Observations

  • The rename is thorough and mechanically consistent across all 17 files
  • Comments, documentation, and code guide are all updated
  • No behavioral changes -- purely a naming fix
  • The diff is clean and easy to verify

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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
@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 77a7926


Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 77a7926


Summary

Clean, well-executed spelling fix with proper backward compatibility handling via kAlias. The OPTIONS file migration path is correct and tested. One missing release note for the public API rename.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Missing release note for public API source-level breaking change — include/rocksdb/advanced_options.h:1367
  • Issue: The struct member memtable_veirfy_per_key_checksum_on_seek in AdvancedColumnFamilyOptions is renamed. This is a source-level breaking change for any downstream C++ code referencing the old member name. There is no entry in unreleased_history/public_api_changes/ documenting this rename.
  • Root cause: Oversight in change documentation.
  • Suggested fix: Add a file like unreleased_history/public_api_changes/fix_memtable_verify_spelling.md with content such as: "Renamed misspelled option memtable_veirfy_per_key_checksum_on_seek to memtable_verify_per_key_checksum_on_seek in AdvancedColumnFamilyOptions. The old spelling is still accepted when parsing OPTIONS files."
M2. db_bench/db_stress flag rename may silently break existing scripts — tools/db_bench_tool.cc:1341, db_stress_tool/db_stress_gflags.cc:1632
  • Issue: The gflags flag --memtable_veirfy_per_key_checksum_on_seek is renamed. Existing scripts or CI configurations using the old flag name will silently fall back to the default value (false) rather than erroring, because gflags ignores unknown flags by default.
  • Root cause: gflags has no built-in alias mechanism for renamed flags.
  • Suggested fix: Document in the release note (see M1). Alternatively a wrapper could map the old name, but likely over-engineering for a spelling fix.

🟢 LOW / NIT

L1. No C/Java API exposure — no action needed
  • The option is not exposed through include/rocksdb/c.h or the Java JNI bindings, so the rename has no impact on those APIs.
L2. claude_md/add_option.md still references old commit hash — NIT
  • The doc references commit 94e65a2e0b4f817aa4bfa4c96cdf867e7980d7bc which used the old spelling. The doc examples are updated to the new spelling but the commit hash still points to the misspelled version. Cosmetically inconsistent but harmless.

Cross-Component Analysis

Context Affected? Assessment
OPTIONS file parsing (old files) Yes Safe. Old name registered as kAlias — parsed correctly, maps to same struct offset.
OPTIONS file serialization Yes Safe. kAlias entries excluded from GetStringFrom* output; only new name serialized.
Old binary reading new OPTIONS Yes Acceptable risk. Old binary won't recognize new name, but this is expected — old binaries also lack any other new options.
WritePreparedTxnDB No Option is CF-level, behavior unchanged.
Java/C API No Not exposed.

Positive Observations

  • Backward compatibility is handled correctly. The kAlias mechanism in cf_options.cc is the established RocksDB pattern for renamed options (same as block_based_table_factory, plain_table_factory, memtable). Old OPTIONS files parse correctly; new serialization uses only the canonical name.

  • Test coverage is good. Three targeted tests cover: new name parsing, old name backward compat via alias, and serialization correctness (old name absent, new name present). Together these implicitly verify the full round-trip migration path.

  • Comprehensive scope. The rename covers all relevant layers: public header, internal options structs, memtable implementation, serialization/deserialization, logging, db_bench, db_stress, crash test, and documentation.


ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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
@meta-codesync

meta-codesync Bot commented Jun 4, 2026

Copy link
Copy Markdown

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D107382276.

@meta-codesync

meta-codesync Bot commented Jun 5, 2026

Copy link
Copy Markdown

@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D107382276.

@meta-codesync

meta-codesync Bot commented Jun 5, 2026

Copy link
Copy Markdown

@xingbowang merged this pull request in 76b8cce.

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

1 participant