Skip to content

ldb dump_wal: add LOG_DATA, TIMED_PUT, PUT_BLOB_INDEX#14211

Open
evanj wants to merge 1 commit intofacebook: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 intofacebook:mainfrom
evanj:evan.jones/ldb-log-data

Conversation

@evanj
Copy link
Copy Markdown
Contributor

@evanj evanj commented Dec 30, 2025

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
Copy link
Copy Markdown
Contributor Author

evanj commented Apr 18, 2026

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant