Skip to content

fix retry oom error in read op#285

Merged
liunyl merged 1 commit into
mainfrom
oom_fix
Dec 11, 2025
Merged

fix retry oom error in read op#285
liunyl merged 1 commit into
mainfrom
oom_fix

Conversation

@liunyl

@liunyl liunyl commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

ReadOperation shohuld not retry when cc req fails with OOM. cc req will retry itself if necessary.

Here are some reminders before you submit the pull request

  • Add tests for the change
  • Document changes
  • Reference the link of issue using fixes eloqdb/tx_service#issue_id
  • Reference the link of RFC if exists
  • Pass ./mtr --suite=mono_main,mono_multi,mono_basic

Summary by CodeRabbit

  • Bug Fixes
    • Simplified out-of-memory error handling for read operations to use standard error flow instead of special retry paths.

✏️ Tip: You can customize this high-level summary in your review settings.

ReadOperation shohuld not retry when cc req fails with OOM. cc req
will retry itself if necessary.
@liunyl liunyl requested a review from yangsw26 December 11, 2025 08:45
@coderabbitai

coderabbitai Bot commented Dec 11, 2025

Copy link
Copy Markdown

Walkthrough

Out-of-memory error handling is simplified in two read operations by removing hash-partition specific retry branches. OOM errors now skip dedicated retry paths and proceed through the standard error handling flow instead of triggering ReRunOp retries.

Changes

Cohort / File(s) Change Summary
OOM retry logic removal
tx_service/src/tx_operation.cpp
Deleted hash-partition OUT_OF_MEMORY retry branch in ReadOperation::Forward; removed out_of_memory_error tracking and associated retry path in BatchReadOperation::Forward. Both now rely on existing error handling flow for OOM scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Hash-partitioned read behavior: Verify that removing the OOM retry branch does not negatively impact recovery in scenarios where hash-partitioned reads encounter memory exhaustion.
  • Fallback error handling: Confirm that the existing error handling flow properly detects and processes OOM errors for both operations without regression.
  • Test coverage: Check that OOM scenarios are adequately covered by existing tests, particularly for BatchReadOperation and hash-partitioned reads.

Poem

🐰 Off with the retry, long may it rest,
Our errors now flow through the standard test.
Simpler paths and fewer threads,
The rabbit hops lighter ahead!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description explains the rationale (cc request will retry itself), but the checklist items are unchecked and no tests, documentation, issue references, or RFC links are provided. Complete the checklist: add tests for OOM handling changes, document the behavior change, reference the related issue using 'fixes eloqdb/tx_service#issue_id', and confirm test suite passes.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix retry oom error in read op' directly relates to the main change: removing OOM retry logic from ReadOperation and BatchReadOperation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch oom_fix

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef4c17b and f0405b7.

📒 Files selected for processing (1)
  • tx_service/src/tx_operation.cpp (0 hunks)
💤 Files with no reviewable changes (1)
  • tx_service/src/tx_operation.cpp

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@liunyl liunyl merged commit 0ee68ce into main Dec 11, 2025
4 checks passed
@liunyl liunyl deleted the oom_fix branch December 11, 2025 12:44
@liunyl liunyl linked an issue Dec 11, 2025 that may be closed by this pull request
This was referenced Dec 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Failed to insert batch: operation failed after 4 retries

2 participants