Fix spelling of memtable_verify_per_key_checksum_on_seek#14811
Fix spelling of memtable_verify_per_key_checksum_on_seek#14811xingbowang wants to merge 3 commits into
Conversation
|
| 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]
|
@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D107382276. |
Codex Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.🟡 Codex Code ReviewAuto-triggered after CI passed — reviewing commit ef6bff9 ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
Claude Code Review - OBSOLETESuperseded by a newer AI review. Expand to see the original review.✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit ef6bff9 SummaryMechanical spelling fix renaming High-severity findings (1):
Full review (click to expand)Findings🔴 HIGHH1. OPTIONS file backwards compatibility broken --
|
| 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
🟡 Codex Code ReviewAuto-triggered after CI passed — reviewing commit 77a7926 ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit 77a7926 SummaryClean, well-executed spelling fix with proper backward compatibility handling via High-severity findings (0): No high-severity findings. Full review (click to expand)Findings🔴 HIGHNone. 🟡 MEDIUMM1. Missing release note for public API source-level breaking change —
|
| 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
kAliasmechanism incf_options.ccis the established RocksDB pattern for renamed options (same asblock_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
|
@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D107382276. |
|
@xingbowang has imported this pull request. If you are a Meta employee, you can view this in D107382276. |
|
@xingbowang merged this pull request in 76b8cce. |
Summary
memtable_veirfy_per_key_checksum_on_seekoption and related flags/config keys tomemtable_verify_per_key_checksum_on_seek.Test Plan
CI