Update submodule for txservice

[xiexiaoy]

eloqdata/tx_service#195

Summary by CodeRabbit

• Refactor
  • Added a configurable option to control point-read behavior on batch-read misses (default unchanged).
  • Replaced pooled object allocation with direct shared allocation for a core service, and added a destruction log.
• Chores
  • Updated an internal transaction service component to align with the new read-handling option.

[coderabbitai]

Walkthrough

Added a local flag point_read_on_miss (false) in EloqRecoveryUnit::batchGetKV and forwarded it to an updated txservice::BatchReadTxRequest constructor. Advanced the tx_service submodule pointer. Removed ObjectPool::newObjectSharedPointer and replaced pooled ServiceStateMachine creation with direct std::make_shared allocation.

Changes

Cohort / File(s)	Summary
Eloq recovery unit change src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp	Added local flag point_read_on_miss = false and passed it as an extra argument to txservice::BatchReadTxRequest in batchGetKV.
tx_service submodule update src/mongo/db/modules/eloq/tx_service/...	Submodule pointer advanced to commit that includes the BatchReadTxRequest constructor signature change (additional bool point_read_on_miss parameter).
ObjectPool API change src/mongo/base/object_pool.h	Removed ObjectPool::newObjectSharedPointer(Args&&... args) overload that returned std::shared_ptr<T>; remaining APIs return unique_ptr variants.
ServiceStateMachine allocation change src/mongo/transport/service_state_machine.cpp	Replaced pooled creation via ObjectPool::newObjectSharedPointer with direct std::make_shared<ServiceStateMachine>; thread-local pool usage commented out/removed and destructor now logs destruction.

Sequence Diagram(s)

sequenceDiagram
    participant ERU as EloqRecoveryUnit
    participant TX as txservice::BatchReadTxRequest
    rect rgb(240,250,240)
    note right of ERU: batchGetKV builds request\nwith explicit flag (default false)
    ERU->>TX: BatchReadTxRequest(..., point_read_on_miss=false)
    TX-->>ERU: constructed request
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Verify all call sites of BatchReadTxRequest updated to new signature.
• Confirm submodule advance contains only the expected constructor change.
• Search for remaining uses of removed ObjectPool::newObjectSharedPointer.
• Review ServiceStateMachine lifecycle and logging impact.

Possibly related PRs

• Update submodules #227 — Advances the same tx_service submodule pointer; likely includes related submodule commits.

Suggested reviewers

• thweetkomputer

Poem

🐇 I nibble a flag, small and bright,
False at first, tucked in the night.
Pools give way to shared embrace,
A quiet hop in code's soft space. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
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.
Title check	❓ Inconclusive	The title 'Update submodule for txservice' is vague and generic, failing to convey the specific nature of the changes across multiple files including removal of a public API method and changes to object allocation patterns.	Consider a more descriptive title that highlights the primary changes, such as 'Remove ObjectPool shared_ptr overload and refactor SSM allocation' or 'Update txservice submodule and refactor object pooling strategy'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 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 30670eb and bc0498f.

📒 Files selected for processing (2)

• src/mongo/base/object_pool.h (0 hunks)
• src/mongo/transport/service_state_machine.cpp (2 hunks)

💤 Files with no reviewable changes (1)

• src/mongo/base/object_pool.h

---

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]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)

468-470: Verify the hardcoded default value.

The point_read_on_miss flag is hardcoded to false, which likely maintains the previous behavior before this parameter was introduced. Please confirm this is the intended default for all batch read operations.

Additionally, consider making the variable const since it's never modified:

   bool isForShare = false;
   bool readLocal = false;
-  bool point_read_on_miss = false;
+  const bool point_read_on_miss = false;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc80a8f and 32e852b.

📒 Files selected for processing (2)

• src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2 hunks)
• src/mongo/db/modules/eloq/tx_service (1 hunks)

🔇 Additional comments (2)

src/mongo/db/modules/eloq/tx_service (1)

1-1: Submodule update is properly coordinated and verified.

Verification confirms all call sites have been updated consistently:

• Single BatchReadTxRequest instantiation at line 471 of eloq_recovery_unit.cpp
• New point_read_on_miss parameter is correctly passed as the final argument
• No lingering references to the old constructor signature exist

The submodule pointer update is safe to proceed.

src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (1)

471-480: LGTM!

The BatchReadTxRequest constructor call correctly includes the new point_read_on_miss parameter, properly adapting to the updated txservice API.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2)

470-470: Consider using camelCase for consistency.

The variable point_read_on_miss uses snake_case, while other boolean variables in the same scope use camelCase (isForWrite, isForShare, readLocal). Consider renaming to pointReadOnMiss for consistency with the existing code style.

---

471-480: LGTM with optional suggestions.

The addition of the point_read_on_miss parameter aligns with the submodule update. The hardcoded false value is consistent with the existing pattern for isForShare and readLocal.

Optional improvements for future consideration:

• Add a comment explaining what point_read_on_miss controls and why it's currently disabled
• If this feature will be configurable later, consider making it a parameter or reading from configuration settings

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32e852b and 23fe64d.

📒 Files selected for processing (2)

• src/mongo/db/modules/eloq/src/eloq_recovery_unit.cpp (2 hunks)
• src/mongo/db/modules/eloq/tx_service (1 hunks)

✅ Files skipped from review due to trivial changes (1)

• src/mongo/db/modules/eloq/tx_service

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/mongo/transport/service_state_machine.cpp (1)

282-306: Remove the dead reset() method from ServiceStateMachine.

The method is not called anywhere in the active codebase. The only reference to ServiceStateMachine::reset() exists in the commented-out object pooling code (line 238), confirming that with the removal of object pooling, this method serves no purpose and should be deleted.

🧹 Nitpick comments (1)

src/mongo/transport/service_state_machine.cpp (1)

233-245: Remove commented-out code.

The commented-out pooling implementation should be removed entirely rather than left in the codebase. If this code needs to be referenced in the future, it's available in version control history.

Apply this diff to remove the dead code:

-    // ServiceStateMachine* ssm{nullptr};
-    // std::unique_ptr<ServiceStateMachine> ssmUptr{nullptr};
-    // bool success = ssmPool.try_dequeue(ssmUptr);
-    // if (success) {
-    //     ssm = ssmUptr.release();
-    //     ssm->Reset(svcContext, std::move(session), transportMode);
-    // } else {
-    //     ssm = new ServiceStateMachine(svcContext, std::move(session), transportMode, group_id);
-    // }
-    // return std::shared_ptr<ServiceStateMachine>(ssm, [](ServiceStateMachine* ptr) {
-    //     ssmPool.enqueue(std::unique_ptr<ServiceStateMachine>(ptr));
-    // });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23fe64d and 5739f0d.

📒 Files selected for processing (2)

• src/mongo/base/object_pool.h (0 hunks)
• src/mongo/transport/service_state_machine.cpp (1 hunks)

💤 Files with no reviewable changes (1)

• src/mongo/base/object_pool.h

🔇 Additional comments (1)

src/mongo/transport/service_state_machine.cpp (1)

230-232: Verified: Change is complete and correct; performance trade-off is intentional.

The transition from thread-local pooling to direct std::make_shared allocation is complete and properly documented. No active pooling code remains, and the design rationale is explicitly stated in the comments: the thread-local pool design conflicts with shared_ptr ownership semantics. The performance overhead from eliminating pooling is an accepted trade-off for correctness and simplified ownership model.