Return error directly for unsupported command#304
Merged
xiexiaoy merged 2 commits intoeloqdata:mainfrom Nov 12, 2025
Merged
Conversation
|
Warning Rate limit exceeded@xiexiaoy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (6)
WalkthroughThis PR marks the dbhash and rename collection commands unsupported for the Eloq storage engine, replaces an assert with guarded error handling in the Eloq KV engine, updates the eloq tx_service submodule pointer, comments out several mapReduce test blocks, and tweaks timezone initialization in ExpressionContext::reset. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CommandLayer
participant EloqEngine
Note over CommandLayer,EloqEngine: New short-circuit behavior for Eloq commands
Client->>CommandLayer: send dbhash/renameCollection Cmd
CommandLayer->>CommandLayer: errmsgRun checks storage engine
alt storage engine == Eloq
CommandLayer-->>Client: return error message (unsupported) `#ffcccb`
else other engine
CommandLayer->>EloqEngine: proceed with normal command flow
EloqEngine-->>CommandLayer: success / failure
CommandLayer-->>Client: final result
end
sequenceDiagram
participant Caller
participant EloqKV
Note over EloqKV: getAllIdents read path adjusted
Caller->>EloqKV: read catalog entry
EloqKV->>EloqKV: check Tx errorCode
alt errorCode != NO_ERROR
EloqKV-->>Caller: log + uassertStatusOK(TxErrorCodeToMongoStatus(...)) `#ffe4b5`
else exists == false
EloqKV-->>Caller: log missing entry and skip
else
EloqKV-->>Caller: process ident
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
100b9f5 to
573575d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/jstests/core/or4.js (1)
74-83: Consider removing commented code or testing for unsupported command behavior.Commented-out code creates maintenance burden and can confuse future readers. Consider one of these alternatives:
- Remove the code entirely if mapReduce will remain permanently unsupported on the Eloq storage engine.
- Add test tags to skip this test on Eloq (e.g.,
// @tags: [requires_mapreduce]).- Test that the command fails gracefully by verifying it returns an appropriate "unsupported" error.
Option 3 provides the most value:
- // assert.eq(5, - // coll.mapReduce( - // function() { - // emit('a', this.a); - // }, - // function(key, vals) { - // return vals.length; - // }, - // {out: {inline: true}, query: {$or: [{a: 2}, {b: 3}]}}) - // .counts.input); + // Verify mapReduce is unsupported on Eloq storage engine + const mapReduceResult = db.runCommand({ + mapReduce: coll.getName(), + map: function() { emit('a', this.a); }, + reduce: function(key, vals) { return vals.length; }, + out: {inline: true}, + query: {$or: [{a: 2}, {b: 3}]} + }); + // Assert command is rejected with appropriate error code for unsupported operations + assert.commandFailedWithCode(mapReduceResult, ErrorCodes.CommandNotSupported);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/mongo/db/commands/dbhash.cpp(3 hunks)src/mongo/db/commands/rename_collection_cmd.cpp(1 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(1 hunks)src/mongo/db/modules/eloq/tx_service(1 hunks)tests/jstests/core/geo_big_polygon3.js(1 hunks)tests/jstests/core/index_stats.js(1 hunks)tests/jstests/core/or4.js(1 hunks)
🧰 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.cpp
🧬 Code graph analysis (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
src/mongo/db/modules/eloq/src/base/eloq_util.cpp (2)
TxErrorCodeToMongoStatus(11-75)TxErrorCodeToMongoStatus(11-11)src/mongo/db/modules/eloq/src/eloq_record_store.cpp (6)
MONGO_LOG(91-119)MONGO_LOG(445-480)MONGO_LOG(520-524)MONGO_LOG(526-532)MONGO_LOG(534-540)MONGO_LOG(542-547)
🔇 Additional comments (4)
src/mongo/db/modules/eloq/tx_service (1)
1-1: Verify submodule commit aligns with PR objectives.Submodule pointer updates can obscure significant changes. Please confirm that commit
54f9ed618c78efa5275e6093c4fb502538d7b749addresses the deadlock response handling mentioned in tx_service#207 and is tested as part of this PR.tests/jstests/core/geo_big_polygon3.js (1)
81-88: LGTM - MapReduce test disabled for Eloq storage engine.The mapReduce test has been properly disabled by commenting out the test block, which aligns with the PR objective to mark mapReduce as unsupported for the Eloq storage engine.
tests/jstests/core/index_stats.js (1)
173-186: LGTM - MapReduce test disabled for Eloq storage engine.The mapReduce test block has been properly disabled, consistent with the PR's objective to mark mapReduce as unsupported for the Eloq storage engine.
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
1181-1189: LGTM - Improved error handling replaces assert.This change replaces an assert with proper error handling that:
- Propagates non-NO_ERROR errors as MongoDB status via
uassertStatusOK- Gracefully skips non-existent catalog entries with logging
This is a critical improvement that prevents crashes when catalog entries are missing or invalid, and is consistent with the error handling pattern used elsewhere in the codebase (e.g.,
eloq_record_store.cpplines 90-118).
Comment on lines
+95
to
+96
| errmsg = "Unsupported feature. The Eloq storage engine does not support dbhash command."; | ||
| return false; |
There was a problem hiding this comment.
Unreachable code after early return.
The early return at line 96 makes all subsequent code in this function (lines 98-209) unreachable. This dead code should be removed to avoid confusion and reduce maintenance burden.
Apply this diff to remove the unreachable code:
virtual bool errmsgRun(OperationContext* opCtx,
const std::string& dbname,
const BSONObj& cmdObj,
std::string& errmsg,
BSONObjBuilder& result) {
errmsg = "Unsupported feature. The Eloq storage engine does not support dbhash command.";
return false;
-
- Timer timer;
-
- std::set<std::string> desiredCollections;
- if (cmdObj["collections"].type() == Array) {
- BSONObjIterator i(cmdObj["collections"].Obj());
- while (i.more()) {
- BSONElement e = i.next();
- if (e.type() != String) {
- errmsg = "collections entries have to be strings";
- return false;
- }
- desiredCollections.insert(e.String());
- }
- }
-
- const std::string ns = parseNs(dbname, cmdObj);
- uassert(ErrorCodes::InvalidNamespace,
- str::stream() << "Invalid db name: " << ns,
- NamespaceString::validDBName(ns, NamespaceString::DollarInDbNameBehavior::Allow));
-
- // We lock the entire database in S-mode in order to ensure that the contents will not
- // change for the snapshot.
- auto lockMode = LockMode::MODE_S;
- auto* session = OperationContextSession::get(opCtx);
- if (session && session->inMultiDocumentTransaction()) {
- // However, if we are inside a multi-statement transaction, then we only need to lock
- // the database in intent mode to ensure that none of the collections get dropped.
- lockMode = getLockModeForQuery(opCtx);
- }
- AutoGetDb autoDb(opCtx, ns, lockMode);
- Database* db = autoDb.getDb();
- std::list<std::string> colls;
- if (db) {
- db->getDatabaseCatalogEntry()->getCollectionNamespaces(&colls);
- colls.sort();
- }
-
- result.append("host", prettyHostName());
-
- md5_state_t globalState;
- md5_init(&globalState);
-
- // A set of 'system' collections that are replicated, and therefore included in the db hash.
- const std::set<StringData> replicatedSystemCollections{"system.backup_users",
- "system.js",
- "system.new_users",
- "system.roles",
- "system.users",
- "system.version",
- "system.views"};
-
- BSONArrayBuilder cappedCollections;
- BSONObjBuilder collectionsByUUID;
-
- BSONObjBuilder bb(result.subobjStart("collections"));
- for (const auto& collectionName : colls) {
-
- NamespaceString collNss(collectionName);
-
- if (collNss.size() - 1 <= dbname.size()) {
- errmsg = str::stream() << "weird fullCollectionName [" << collNss.toString() << "]";
- return false;
- }
-
- // Only include 'system' collections that are replicated.
- bool isReplicatedSystemColl =
- (replicatedSystemCollections.count(collNss.coll().toString()) > 0);
- if (collNss.isSystem() && !isReplicatedSystemColl)
- continue;
-
- if (collNss.coll().startsWith("tmp.mr.")) {
- // We skip any incremental map reduce collections as they also aren't replicated.
- continue;
- }
-
- if (desiredCollections.size() > 0 &&
- desiredCollections.count(collNss.coll().toString()) == 0)
- continue;
-
- // Don't include 'drop pending' collections.
- if (collNss.isDropPendingNamespace())
- continue;
-
- if (Collection* collection = db->getCollection(opCtx, collectionName)) {
- if (collection->isCapped()) {
- cappedCollections.append(collNss.coll());
- }
-
- if (OptionalCollectionUUID uuid = collection->uuid()) {
- uuid->appendToBuilder(&collectionsByUUID, collNss.coll());
- }
- }
-
- // Compute the hash for this collection.
- std::string hash = _hashCollection(opCtx, db, collNss.toString());
-
- bb.append(collNss.coll(), hash);
- md5_append(&globalState, (const md5_byte_t*)hash.c_str(), hash.size());
- }
- bb.done();
-
- result.append("capped", BSONArray(cappedCollections.done()));
- result.append("uuids", collectionsByUUID.done());
-
- md5digest d;
- md5_finish(&globalState, d);
- std::string hash = digestToString(d);
-
- result.append("md5", hash);
- result.appendNumber("timeMillis", timer.millis());
-
- return 1;
}🤖 Prompt for AI Agents
In src/mongo/db/commands/dbhash.cpp around lines 95 to 96, there is an early
return that sets errmsg and returns false which renders the subsequent code
(lines 98-209) unreachable; remove the dead/unreachable code after that return
(or alternatively remove the early return and properly implement behavior) so
the function contains only reachable logic—delete the unreachable block and
ensure the function compiles and behaves as intended after removal.
Comment on lines
+97
to
+100
| errmsg = | ||
| "Unsupported feature. The Eloq storage engine does not support rename collection " | ||
| "command."; | ||
| return false; |
There was a problem hiding this comment.
Unreachable code after early return.
The early return at line 100 makes all subsequent code in this function (lines 102-167) unreachable. This dead code should be removed to avoid confusion and reduce maintenance burden.
Apply this diff to remove the unreachable code:
virtual bool errmsgRun(OperationContext* opCtx,
const string& dbname,
const BSONObj& cmdObj,
string& errmsg,
BSONObjBuilder& result) {
errmsg =
"Unsupported feature. The Eloq storage engine does not support rename collection "
"command.";
return false;
-
- const auto sourceNsElt = cmdObj[getName()];
- const auto targetNsElt = cmdObj["to"];
-
- uassert(ErrorCodes::TypeMismatch,
- "'renameCollection' must be of type String",
- sourceNsElt.type() == BSONType::String);
- uassert(ErrorCodes::TypeMismatch,
- "'to' must be of type String",
- targetNsElt.type() == BSONType::String);
-
- const NamespaceString source(sourceNsElt.valueStringData());
- const NamespaceString target(targetNsElt.valueStringData());
-
- uassert(ErrorCodes::InvalidNamespace,
- str::stream() << "Invalid source namespace: " << source.ns(),
- source.isValid());
- uassert(ErrorCodes::InvalidNamespace,
- str::stream() << "Invalid target namespace: " << target.ns(),
- target.isValid());
-
- if ((repl::ReplicationCoordinator::get(opCtx)->getReplicationMode() !=
- repl::ReplicationCoordinator::modeNone)) {
- if (source.isOplog()) {
- errmsg = "can't rename live oplog while replicating";
- return false;
- }
- if (target.isOplog()) {
- errmsg = "can't rename to live oplog while replicating";
- return false;
- }
- }
-
- if (source.isOplog() != target.isOplog()) {
- errmsg = "If either the source or target of a rename is an oplog name, both must be";
- return false;
- }
-
- Status sourceStatus = userAllowedWriteNS(source);
- if (!sourceStatus.isOK()) {
- errmsg = "error with source namespace: " + sourceStatus.reason();
- return false;
- }
-
- Status targetStatus = userAllowedWriteNS(target);
- if (!targetStatus.isOK()) {
- errmsg = "error with target namespace: " + targetStatus.reason();
- return false;
- }
-
- if (source.isSystemDotIndexes() || target.isSystemDotIndexes()) {
- errmsg = "renaming system.indexes is not allowed";
- return false;
- }
-
- if (source.isServerConfigurationCollection()) {
- uasserted(ErrorCodes::IllegalOperation,
- "renaming the server configuration "
- "collection (admin.system.version) is not "
- "allowed");
- }
-
- RenameCollectionOptions options;
- options.dropTarget = cmdObj["dropTarget"].trueValue();
- options.stayTemp = cmdObj["stayTemp"].trueValue();
- uassertStatusOK(renameCollection(opCtx, source, target, options));
- return true;
}🤖 Prompt for AI Agents
In src/mongo/db/commands/rename_collection_cmd.cpp around lines 97 to 100, the
function sets errmsg and returns false at line 100, which makes the subsequent
code (lines 102–167) unreachable; remove the dead code block from lines 102
through 167 so only the intended early-return path remains and update any
related comments or includes if they become unused.
5eac673 to
ff27be6
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)
src/mongo/db/pipeline/expression_context.cpp(1 hunks)tests/jstests/core/geo_big_polygon3.js(1 hunks)tests/jstests/core/index_stats.js(1 hunks)tests/jstests/core/or4.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/jstests/core/or4.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/jstests/core/index_stats.js
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🔇 Additional comments (1)
src/mongo/db/pipeline/expression_context.cpp (1)
87-89: LGTM! Improved consistency and correctness.This change properly initializes
timeZoneDatabasefrom the service context whenopCtxis available, aligning with the constructor (lines 113-115) and the other reset method (lines 139-141). Previously, this reset method likely set it tonullptrunconditionally, which would break timezone-related aggregation operations after reset when a valid service context was present.
lzxddz
approved these changes
Nov 12, 2025
ff27be6 to
c866ca1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
eloqdata/tx_service#207
Summary by CodeRabbit
Bug Fixes
Tests
Chores