try to root cause the failure

[xingbowang]

No description provided.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 303.0s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit f7911c4

---

Summary

This is a debugging/investigation PR ("try to root cause the failure") that isolates the ARM CI job, expands its test scope, and fixes a test service bug where early OpenAndCompact() failures left the result string empty, causing the primary to lose the underlying error status. The test fix is correct and should be permanent; the CI changes must not be merged.

High-severity findings (1):

• [pr-jobs.yml:7] ONLY_JOB: 'build-linux-arm' disables all other CI jobs -- must be reverted before merge.

Full review (click to expand)

Findings

🔴 HIGH

H1. CI debug settings must not be merged — pr-jobs.yml:7

• Issue: ONLY_JOB: 'build-linux-arm' causes all CI jobs except build-linux-arm to be skipped. If merged, the full CI matrix (Linux x86_64, macOS, Windows, ASAN/TSAN, Java, etc.) will stop running on all PRs.
• Root cause: Intentional debug isolation, but dangerous if committed to main.
• Suggested fix: Revert ONLY_JOB to '' before merging. Consider whether the libgflags-dev and make check changes to the ARM job should be kept (see M1, M2).

🟡 MEDIUM

M1. ARM job expanded from subset to full make check — pr-jobs.yml:768

• Issue: The ARM job previously ran only platform-dependent tests (ROCKSDBTESTS_PLATFORM_DEPENDENT=only make V=1 J=4 -j4 all_but_some_tests check_some). The change runs make V=1 J=4 -j4 check which is the full test suite. On a 4-core ARM runner this may significantly increase CI time or cause timeouts.
• Suggested fix: If the intent is to permanently expand ARM test coverage, validate that the full suite completes within the CI timeout. Otherwise, revert to the original command.

M2. libgflags-dev addition may be intentionally permanent — pr-jobs.yml:764

• Issue: Adding libgflags-dev is needed for running the full test suite (many tests use gflags). This is consistent with other CI jobs (e.g., line 400 installs it). However, the original ARM config intentionally omitted it, similar to how the MinGW job at line 269 explicitly removes it.
• Suggested fix: If keeping make check for ARM, keep libgflags-dev. If reverting to the original test scope, remove it.

M3. Second cancel scenario assertion inconsistency — compaction_service_test.cc:1591

• Issue: The "cancel in progress" scenario at line 1591 still uses ASSERT_TRUE(s.IsIncomplete()) while the "cancel at beginning" was updated to IsManualCompactionPaused(). Both paths produce Status::Incomplete(kManualCompactionPaused). In the "in progress" case, cancellation fires during CompactionJob::Run() (compaction_job.cc:1870), the result IS serialized, and the primary extracts ManualCompactionPaused. Line 1591 should also use the more precise assertion.
• Suggested fix: ASSERT_TRUE(s.IsManualCompactionPaused()) << s.ToString();

🟢 LOW / NIT

L1. #include <cstdio> addition is correct — compaction_service_test.cc:6

• Needed for fprintf. Follows "include what you use" principle.

L2. fprintf(stderr, ...) acceptable for test code — compaction_service_test.cc:157

• Unusual but acceptable since the test service doesn't have a convenient logger reference.

L3. Error serialization fix should be permanent — compaction_service_test.cc:152-161

• The fix addresses a real bug: DB::OpenAndCompact() has many early-return paths where output stays empty. The primary at compaction_service_job.cc:169-185 handles this but loses the specific error. Consider documenting this requirement in the CompactionService API or having OpenAndCompact() itself serialize errors for early failures.

Cross-Component Analysis

Context	Affected?	Analysis
Primary-side kFailure handling	YES	With fix: CompactionServiceResult::Read() succeeds, primary extracts ManualCompactionPaused (line 178). Without: gets generic Incomplete.
result->empty() guard	SAFE	All early-return paths in OpenAndCompact() (lines 1317-1421) return before compaction_result.Write(output) at line 1446.
Write() failure path	SAFE	Falls back to pre-fix behavior (empty result, generic error). fprintf adds diagnostics.

Positive Observations

• IsManualCompactionPaused() is strictly more precise than IsIncomplete() (subset check).
• << s.ToString() on assertion provides diagnostic output on failure.
• The result->empty() guard correctly identifies the gap between OpenAndCompact() behavior and primary expectations.

---

ℹ️ 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