ClearTx also clears ReaderWriterCntl

[MrGuin]

Fixes https://github.com/eloqdata/project_tracker/issues/186.

Summary by CodeRabbit

• Refactor

  • Enhanced internal transaction conflict detection with additional diagnostic logging.
  • Improved transaction cleanup logic for schema write locks during abort operations.
  • Refined transaction number handling for consistency across conflict resolution paths.

• Chores

  • Added diagnostic error logging for transaction acquisition failures to improve observability.

[coderabbitai]

Warning

Rate limit exceeded

@MrGuin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 50 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 2c9045c and b6afb45.

📒 Files selected for processing (1)

• tx_service/src/tx_operation.cpp

Walkthrough

This pull request adds diagnostic and warning logging across multiple transaction and concurrency control components. Changes include logging in conflict detection paths for acquire operations, enhanced diagnostics for writer transaction conflicts, and schema control cleanup during transaction abort. No control flow or behavioral changes are introduced.

Changes

Cohort / File(s)	Summary
Catalog and Template Concurrency Control Headers tx_service/include/cc/catalog_cc_map.h, tx_service/include/cc/template_cc_map.h	Added warning logs in AcquireAllCc conflict handling paths to emit transaction, table, and key details before returning write conflict errors.
Core Concurrency Control Implementation tx_service/src/cc/cc_shard.cpp, tx_service/src/cc/reader_writer_cntl.cpp	Introduced schema control cleanup logic in ClearTx for native node groups; refactored AddWriter to consistently derive and reuse requester transaction number for conflict detection with enhanced logging.
Transaction Operation Diagnostics tx_service/src/tx_operation.cpp	Added error-level diagnostic logging in AcquireAllOp::Forward to output index, error code, and error message for individual failed acquire results.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• Range read block on rw conflict #367: Adds diagnostic logging to AcquireAllOp::Forward with overlapping code-level changes.
• store writer txn instead of write_req when write_req is processed #192: Modifies AddWriter transaction derivation and conflict logging in reader_writer_cntl, directly related to writer handling changes.
• obj_cmd_op read catalog UpsertLockHodlingTxn in debug build #154: Enhances AcquireAllCc logging in catalog concurrency control paths.

Suggested reviewers

• thweetkomputer
• githubzilla
• liunyl

Poem

🐰 Hop, log, and trace the way,
Conflicts caught in warning's play,
Schema cleanup, transactions mend,
Diagnostics help us comprehend! ✨

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is minimal and incomplete, providing only a GitHub issue reference without explaining the changes, testing status, or following the repository's description template.	Expand the description to explain what ClearTx changes were made, why they were necessary, confirm test execution results, and follow the template checklist items.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'ClearTx also clears ReaderWriterCntl' directly corresponds to the main change in the PR, which introduces cleanup of ReaderWriterCntl during transaction abort in ClearTx.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix_rw_cntl

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

tx_service/src/tx_operation.cpp (1)

1910-1912: Use non-error severity and include txn context for this conflict log.

Line 1910 logs every acquire-all failure as ERROR; many of these are expected retry/conflict outcomes and can create noisy error telemetry. Prefer WARNING (or sampled warning) and include tx_number for correlation.

Proposed change

-                    LOG(ERROR) << "AcquireAll of idx: " << idx
-                               << " err code: " << int(hd_result.ErrorCode())
-                               << ", err msg: " << hd_result.ErrorMsg();
+                    LOG(WARNING) << "AcquireAll failed, tx_number: "
+                                 << txm->TxNumber() << ", idx: " << idx
+                                 << ", err code: "
+                                 << static_cast<int>(hd_result.ErrorCode())
+                                 << ", err msg: " << hd_result.ErrorMsg();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tx_service/src/tx_operation.cpp` around lines 1910 - 1912, The log at the
AcquireAll failure site currently uses LOG(ERROR) and lacks transaction context;
change the severity to LOG(WARNING) (or use a sampled warning mechanism) and
include the transaction identifier (tx_number) in the message for correlation,
preserving the existing details (idx, int(hd_result.ErrorCode()),
hd_result.ErrorMsg()); update the log call referencing the same AcquireAll
failure location where LOG(ERROR) and hd_result are used so the message becomes
a warning with tx_number included.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tx_service/src/tx_operation.cpp`:
- Around line 1910-1912: The log at the AcquireAll failure site currently uses
LOG(ERROR) and lacks transaction context; change the severity to LOG(WARNING)
(or use a sampled warning mechanism) and include the transaction identifier
(tx_number) in the message for correlation, preserving the existing details
(idx, int(hd_result.ErrorCode()), hd_result.ErrorMsg()); update the log call
referencing the same AcquireAll failure location where LOG(ERROR) and hd_result
are used so the message becomes a warning with tx_number included.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 962933b and 2c9045c.

📒 Files selected for processing (5)

• tx_service/include/cc/catalog_cc_map.h
• tx_service/include/cc/template_cc_map.h
• tx_service/src/cc/cc_shard.cpp
• tx_service/src/cc/reader_writer_cntl.cpp
• tx_service/src/tx_operation.cpp