Skip to content

ldb dump_wal: add LOG_DATA, TIMED_PUT, PUT_BLOB_INDEX#14211

Open
evanj wants to merge 1 commit into
facebook:mainfrom
evanj:evan.jones/ldb-log-data
Open

ldb dump_wal: add LOG_DATA, TIMED_PUT, PUT_BLOB_INDEX#14211
evanj wants to merge 1 commit into
facebook:mainfrom
evanj:evan.jones/ldb-log-data

Conversation

@evanj

@evanj evanj commented Dec 30, 2025

Copy link
Copy Markdown
Contributor

Previously, LOG_DATA would be ignored and would print like an empty record, and TIMED_PUT and PUT_BLOB_INDEX would cause ldb to exit with an unimplemented error.

Add a unit test to cover these cases.

@meta-cla meta-cla Bot added the CLA Signed label Dec 30, 2025
@evanj evanj force-pushed the evan.jones/ldb-log-data branch 2 times, most recently from 3fd14b6 to 9d968fe Compare December 31, 2025 12:18
Previously, LOG_DATA would be ignored and would print like an empty
record, and TIMED_PUT and PUT_BLOB_INDEX would cause ldb to exit
with an unimplemented error.

Add a unit test to cover these cases.
@evanj evanj force-pushed the evan.jones/ldb-log-data branch from 9d968fe to 2072d5c Compare April 18, 2026 13:39
@evanj

evanj commented Apr 18, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on main. This is still relevant.

@github-actions

Copy link
Copy Markdown

✅ clang-tidy: No findings on changed lines

Completed in 341.2s.

@github-actions

Copy link
Copy Markdown

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 2072d5c


Code Review: ldb dump_wal: add LOG_DATA, TIMED_PUT, PUT_BLOB_INDEX

APPROVE with suggestions. The implementation is correct, follows existing patterns, and solves a real user-facing problem.

Findings

F1. Test only asserts LOG_DATA — missing TIMED_PUT and PUT_BLOB_INDEX assertions (MEDIUM)

The test writes all three record types but only verifies LOG_DATA. The other two handlers could be broken and the test would still pass. Add:

ASSERT_NE(std::string::npos, captured_output.find("TIMED_PUT("))
    << "ldb output:\n\n" << captured_output;
ASSERT_NE(std::string::npos, captured_output.find("PUT_BLOB_INDEX("))
    << "ldb output:\n\n" << captured_output;

F2. stdout capture not restored on early test exit (LOW)

The rdbuf swap pattern is not exception/early-return safe. An RAII guard would be more robust. This is a novel pattern in the RocksDB test suite.

F3. TimedPutCF format includes write_time in parentheses (SUGGESTION)

TIMED_PUT(cf, write_time) is mildly inconsistent with PUT(cf), MERGE(cf), etc. However, COMMIT_WITH_TIMESTAMP(xid, ts) provides precedent. Current format is acceptable.

F4. Consider decoding blob index for readability (SUGGESTION)

BlobIndex::DebugString() could provide human-readable output instead of raw hex. Nice-to-have, not required.

Full review written to review-findings.md.


ℹ️ 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants