Acquire write intent for base table only#269
Conversation
WalkthroughAdds two tx error → Mongo error mappings and capitalizes a log prefix; removes rsync verbose flag in two build scripts; advances an eloq tx_service submodule pointer; makes cursor seek boundary and isForWrite logic explicit; and sets RocksDB query worker count to 10× reserved threads. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant EloqUtil
participant MongoError
Caller->>EloqUtil: reportTxError(txErr)
EloqUtil->>EloqUtil: switch(txErr)
note right of EloqUtil `#FFF3E0`: new cases: READ_WRITE_CONFLICT, REQUESTD_TABLE_NOT_EXISTS
EloqUtil->>MongoError: map to SnapshotUnavailable / NamespaceNotFound
MongoError-->>Caller: return mapped Status
sequenceDiagram
autonumber
participant Caller
participant EloqIndexCursor
participant IndexScan
Caller->>EloqIndexCursor: _seekCursor(startInclusive, upsert, ...)
EloqIndexCursor->>EloqIndexCursor: endInclusive = false
EloqIndexCursor->>EloqIndexCursor: isForWrite = upsert && _indexName->IsBase()
EloqIndexCursor->>IndexScan: indexScanOpen(startKey, startInclusive, endKey, endInclusive, isForWrite)
IndexScan-->>EloqIndexCursor: scan handle / status
EloqIndexCursor-->>Caller: positioned / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (3)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/base/eloq_util.cpp (1)
62-64: Minor typo in external error code constant name.The error code constant appears to have a typo:
REQUESTD_TABLE_NOT_EXISTS(missing 'E' in "REQUESTED"). If this constant is defined in the externaltxservicelibrary, it cannot be fixed here, but it's worth noting. The mapping toErrorCodes::NamespaceNotFoundis semantically correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mongo/db/modules/eloq/src/base/eloq_util.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
startInclusive(557-587)startInclusive(557-557)
🔇 Additional comments (2)
src/mongo/db/modules/eloq/src/eloq_index.cpp (2)
277-277: Variable extraction improves clarity.Extracting the
endInclusiveflag makes theindexScanOpencall more explicit, even though it's currently hard-coded tofalse. The comment on line 279 clarifies that end-inclusive semantics are governed by_endPosition.
278-288: Change is correct and follows established patterns.The modification at line 278 properly restricts write intent to base indexes only:
isForWrite = _opCtx->isUpsert() && _indexName->IsBase(). This aligns with the PR objective and is consistent with the codebase patterns.The _idRead method at line 238 correctly omits the base index check because ID indexes are always base indexes (they serve as the primary key), making the check redundant. Similarly, record store operations on primary tables (lines 497, 576, 729 in eloq_record_store.cpp) omit the check for the same reason. The pattern is sound: explicit base index checks appear only where indexes can be either base or secondary.
4626ace to
4bd9e34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
concourse/scripts/build_tarball.bash(1 hunks)concourse/scripts/build_tarball_open.bash(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- concourse/scripts/build_tarball.bash
🧰 Additional context used
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_index.cpp (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (2)
startInclusive(557-587)startInclusive(557-557)
🔇 Additional comments (2)
concourse/scripts/build_tarball_open.bash (1)
137-137: LGTM - Cleaner build output.Removing the verbose flag (
-v) from rsync reduces build log noise while preserving all functional behavior (archive mode, symlink dereferencing, and ignore-existing).src/mongo/db/modules/eloq/src/base/eloq_util.cpp (1)
62-64: Approved - error mapping is semantically correct.The enum value
REQUESTD_TABLE_NOT_EXISTSis from the externaltxservicelibrary (not in this repository). The code compiles successfully, confirming the enum name matches the actual definition in txservice. The mapping toErrorCodes::NamespaceNotFoundis semantically appropriate regardless of the enum spelling.
32b49e8 to
0c71eb3
Compare
c1573a3 to
75085ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
647-647: Consider making the multiplier configurable and add overflow protection.The hardcoded
10multiplier lacks documentation of its rationale. Additionally, multiplying without overflow checks could theoretically produce incorrect values ifreservedThreadNumis very large.Consider:
- Extracting the multiplier to a named constant or configuration option
- Adding a reasonable upper bound or overflow check
Example:
+constexpr uint32_t kQueryWorkerMultiplier = 10; +constexpr uint32_t kMaxQueryWorkers = 10000; // or appropriate limit + INIReader fake_config_reader(nullptr, 0); EloqDS::RocksDBConfig rocksdb_config(fake_config_reader, _dbPath); -rocksdb_config.query_worker_num_ = 10 * serverGlobalParams.reservedThreadNum; +uint32_t desired_workers = kQueryWorkerMultiplier * serverGlobalParams.reservedThreadNum; +rocksdb_config.query_worker_num_ = std::min(desired_workers, kMaxQueryWorkers);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
concourse/scripts/build_tarball.bash(1 hunks)concourse/scripts/build_tarball_open.bash(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- concourse/scripts/build_tarball_open.bash
- src/mongo/db/modules/eloq/tx_service
- src/mongo/db/modules/eloq/src/eloq_record_store.cpp
- src/mongo/db/modules/eloq/src/eloq_index.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
src/mongo/db/modules/eloq/src/base/eloq_util.cppsrc/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
🔇 Additional comments (3)
concourse/scripts/build_tarball.bash (1)
172-172: Approved: rsync verbose flag removal reduces build log verbosity.The removal of the
-vflag maintains all essential rsync behavior: archive mode (-a), symlink following (-L), and skip-existing files (--ignore-existing). This aligns with the stated PR objective and mirrors similar adjustments inbuild_tarball_open.bash.src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
15-15: LGTM: Consistent capitalization.The capitalization change aligns with treating "Eloq" as a proper noun.
65-67: LGTM: Appropriate error mapping for non-existent table.The mapping of
REQUESTD_TABLE_NOT_EXISTStoNamespaceNotFoundis semantically correct and the case properly terminates withbreak.Note: The enum name appears to contain a typo ("REQUESTD" instead of "REQUESTED"), but this likely originates from the
tx_servicesubmodule and should be addressed there if it's indeed an error.
75085ab to
6eba968
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_record_store.cpp (1)
576-587: LGTM: Explicit boundary handling improves clarity.Making the seek boundaries explicit is a good refactor. The
startInclusiveparameter now properly controls the start boundary, whileendInclusiveis explicitly set tofalsefor the end boundary. This improves code readability and maintainability.If
endInclusiveis alwaysfalse, consider documenting why the end boundary is always non-inclusive or whether future use cases might require it to be configurable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
concourse/scripts/build_tarball.bash(1 hunks)concourse/scripts/build_tarball_open.bash(1 hunks)src/mongo/db/modules/eloq/src/base/eloq_util.cpp(3 hunks)src/mongo/db/modules/eloq/src/eloq_index.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_record_store.cpp(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- concourse/scripts/build_tarball_open.bash
- src/mongo/db/modules/eloq/src/eloq_index.cpp
- concourse/scripts/build_tarball.bash
- src/mongo/db/modules/eloq/tx_service
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
📚 Learning: 2025-09-25T12:24:06.434Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 211
File: src/mongo/db/modules/eloq/cmake/build_eloq_log_service.cmake:26-80
Timestamp: 2025-09-25T12:24:06.434Z
Learning: The user githubzilla implemented a fix in commit fe98aaf to address the MEMORY state incorrectly triggering RocksDB discovery in build_eloq_log_service.cmake. This was a control flow issue where MEMORY builds were falling into an else branch that still attempted RocksDB discovery.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cppsrc/mongo/db/modules/eloq/src/base/eloq_util.cpp
🔇 Additional comments (3)
src/mongo/db/modules/eloq/src/base/eloq_util.cpp (3)
15-15: LGTM: Improved consistency.Capitalizing "Eloq" improves log message consistency.
32-34: LGTM: Appropriate error mapping for read-write conflicts.Mapping
READ_WRITE_CONFLICTtoSnapshotUnavailableis correct for transient transaction errors. MongoDB's retry logic handlesSnapshotUnavailableas a retriable error, which aligns with the expected behavior for read-write conflicts.
64-66: Verify the TxErrorCode enum definition in the tx_service submodule.The enum value
REQUESTD_TABLE_NOT_EXISTSis used in eloq_util.cpp:64 but the TxErrorCode enum definition from the tx_service module is not accessible in the codebase (likely a git submodule). Manually verify whether this enum value is intentionally namedREQUESTD_TABLE_NOT_EXISTSor if the typo exists in the upstream tx_service enum definition. If the typo exists there, it should be fixed in the enum definition and all references updated accordingly.
| // "/home/lzx/test-eloqsql/eloq_ds.ini"; | ||
| INIReader fake_config_reader(nullptr, 0); | ||
| EloqDS::RocksDBConfig rocksdb_config(fake_config_reader, _dbPath); | ||
| rocksdb_config.query_worker_num_ = 10 * serverGlobalParams.reservedThreadNum; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the 10× multiplier for query workers.
The hard-coded multiplier of 10 for RocksDB query workers could lead to resource contention if reservedThreadNum is large. Consider adding bounds checking or documenting why this specific ratio is appropriate for the RocksDB query workload.
🏁 Script executed:
#!/bin/bash
# Search for other worker thread configurations to compare multipliers
rg -n -C3 'query_worker_num_|worker.*num.*=' --type=cpp | head -50Length of output: 790
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for bounds checking, validation, or comments around query_worker_num_
rg -n -B5 -A5 'query_worker_num_' --type=cpp
Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Search for reservedThreadNum definition and any bounds/validation
rg -n -B3 -A3 'reservedThreadNum' --type=cpp | head -100Length of output: 7588
🏁 Script executed:
#!/bin/bash
# Look for documentation or comments explaining thread multipliers in RocksDB config
rg -n 'worker|thread' src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp -C5 | grep -E '(//|/\*|\*/)' | head -20Length of output: 1199
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for RocksDBConfig class definition to see if query_worker_num_ has built-in validation/limits
rg -n 'class RocksDBConfig|query_worker_num_' src/mongo/db/modules/eloq/ -A10 -B2 | head -80
Length of output: 1469
🏁 Script executed:
#!/bin/bash
# Search for any documentation or design comments around the 10x multiplier in commit history or comments
rg -n '10.*worker|worker.*10|query.*worker' src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp -B5 -A5Length of output: 1260
Add documentation and bounds checking for query_worker_num_ calculation.
The hard-coded 10× multiplier lacks justification and creates unbounded thread growth. Since reservedThreadNum has no documented upper limit, this could lead to thread explosion under high core count scenarios. Either document why 10× is appropriate for RocksDB query workload characteristics, or add bounds checking to cap the resulting thread count.
🤖 Prompt for AI Agents
In src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp around line 647, the
assignment rocksdb_config.query_worker_num_ = 10 *
serverGlobalParams.reservedThreadNum uses a hard-coded 10× multiplier without
documentation or bounds checking; update this to (1) add a short comment
explaining why a multiplier is used (e.g., expected RocksDB query
I/O/concurrency characteristics) and (2) clamp the computed value to a safe
range to prevent thread explosion (for example, enforce a minimum of 4 and a
conservative maximum such as 128 or a value derived from
serverGlobalParams.maxThreads if available). Implement the clamp so
query_worker_num_ = clamp(10 * reservedThreadNum, minWorkers, maxWorkers) and
include a comment noting the chosen bounds and rationale.
d93fe72 to
7f47fb2
Compare
7f47fb2 to
722be08
Compare
eloqdata/tx_service#173
Summary by CodeRabbit
Bug Fixes
Chores