Skip to content

fix ReadLocalOp assert#331

Merged
MrGuin merged 1 commit into
mainfrom
fix_read_local
Dec 30, 2025
Merged

fix ReadLocalOp assert#331
MrGuin merged 1 commit into
mainfrom
fix_read_local

Conversation

@MrGuin

@MrGuin MrGuin commented Dec 30, 2025

Copy link
Copy Markdown
Collaborator

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
    • Enhanced error handling for transaction operations when leader nodes are unavailable, enabling more consistent error recovery and retry behavior across different error conditions.

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

@coderabbitai

coderabbitai Bot commented Dec 30, 2025

Copy link
Copy Markdown

Walkthrough

This change extends error handling in ReadLocalOperation::Forward within tx_operation.cpp by treating LEADER_NODE_UNREACHABLE consistently with existing critical errors (DATA_STORE_ERR and ACQUIRE_KEY_LOCK_FAILED_FOR_RW_CONFLICT), modifying control flow to apply the same retry or abort logic to this additional error code.

Changes

Cohort / File(s) Summary
Error handling expansion in ReadLocalOperation
tx_service/src/tx_operation.cpp
Added LEADER_NODE_UNREACHABLE error code to existing conditional error handling block, aligning leader-unreachability handling with existing failure modes

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Possibly related PRs

Suggested reviewers

  • yi-xmu
  • liunyl

Poem

🐰 A leader unreached, now caught with care,
With sibling errors in the handling pair,
Retry or abort, the path is clear,
The rabbit hops on, no more to fear! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description contains only an uncompleted checklist template with no actual details about the changes, objectives, testing status, or issue references. Add substantive description of the fix, explain why the assertion was problematic, document testing performed, and reference any related issues using 'fixes eloqdb/tx_service#issue_id'.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix ReadLocalOp assert' is specific and directly related to the main change—fixing error handling in ReadLocalOperation, which addresses an assertion issue.
✨ 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 fix_read_local

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
tx_service/src/tx_operation.cpp (1)

292-329: Assert now correctly matches the error modes this branch can see

Extending the assert to allow DATA_STORE_ERR and LEADER_NODE_UNREACHABLE is consistent with how other read paths treat these errors (retry/abort with the same logic) and should eliminate the spurious ReadLocalOperation assertion without changing behavior.

You might optionally broaden the comment starting at Line 309, since the branch now also covers data-store and leader‑reachability failures, not just DDL‑blocked range locks.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca396ca and ea2c4b8.

📒 Files selected for processing (1)
  • tx_service/src/tx_operation.cpp

@MrGuin MrGuin merged commit 755c4b7 into main Dec 30, 2025
4 checks passed
@MrGuin MrGuin deleted the fix_read_local branch December 30, 2025 11:12
This was referenced Jan 4, 2026
@coderabbitai coderabbitai Bot mentioned this pull request Jan 17, 2026
5 tasks
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.

2 participants