[SERVER-48618] Remove TODO's for SERVER-48618 Created: 05/Jun/20  Updated: 29/Oct/23  Resolved: 18/Sep/20

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 4.8.0

Type: Bug Priority: Major - P3
Reporter: Haley Connelly Assignee: Haley Connelly
Resolution: Fixed Votes: 0
Labels: sharding-wfbf-day
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-31946 Doing a find by UUID with a shardVers... Backlog
is related to SERVER-47532 ShardServerProcessInterface: Convert ... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Sharding 2020-07-27, Sharding 2020-08-10, Sharding 2020-08-24, Sharding 2020-09-21, Sharding 2020-10-05
Participants:

 Description   

new description

We've decided not to do what this ticket originally aimed to do. Remove TODO's for this ticket in the codebase.

old description
Passing an empty NamespaceString to OperationShardingState::initializeClientRoutingVersionsFromCommand()   means the OperationShardingState cannot correctly store the shard/database version tied to the command.

service_entry_point_common execCommandDatabase calls oss.initializeClientRoutingVersionsFromCommand(invocation->ns(), request.body).

However, not all commands that go through this path are derived from InvocationBase - thus invocation->ns() yields an empty NamespaceString. 

List of commands we found that pass an empty NamespaceString to OperationShardingState::initializeClientRoutingVersionsFromCommand() that aren't derived from InvocationBase:

Additionally, DropConnectionsCmd derives from InvocationBase but passes an empty NamespaceString to OperationShardingState::initializeClientRoutingVersionsFromCommand().

 



 Comments   
Comment by Githook User [ 18/Sep/20 ]

Author:

{'name': 'Haley Connelly', 'email': 'haley.connelly@mongodb.com', 'username': 'haleyConnelly'}

Message: SERVER-48618 Remove TODO's for SERVER-48618
Branch: master
https://github.com/mongodb/mongo/commit/0a6cb53bd220ceaae946ff5b1a4ddb51d0582348

Comment by Haley Connelly [ 17/Sep/20 ]

After some discussion, we've decided not to move forward with the ticket, and are just going to remove TODO's for it instead.

Comment by Haley Connelly [ 08/Sep/20 ]

kaloian.manassiev, we discovered a scenario where the OSS tracks an incomplete/ inconsistent namespace, but don't have a good solution for its fix 

Initializing OSS with incomplete namespace * mongos specifies the UUID rather than the collection name, and explicitly specifies shard version UNSHARDED, to the find command when fetching the post image for unsharded collections (eg: in change streams).

Why the behavior isn't broken today despite OSS inconsistency
(1) Issue FindCmd with UUID for dbName, explicitly send UNSHARDED with cmd.
(2) OSS maps dbName to UNSHARDED
(3) FindCmd constructs AutoGetCollectionForReadCommand with full namespace
(4) AutoGetCollectionForReadCommand's constructor calls css->checkShardVersionOrThrow() and eventually leads to CollectionShardingRuntime::_getMetadataWithVersionCheckAt
(5) getOperationRecievedVersion, returns boost::none rather than UNSHARDED because it is called with the full nss (not just dbName and OSS doesn't have a matching entry in _shardVersions).
(4) collection perceived as UNSHARDED anyway

Comment by Haley Connelly [ 25/Aug/20 ]

Yes, it is possible that the user can provide an empty namespace (it would fail eventually, but with the invariant in place, invariant first).

Max brought up a good suggestion offline - if both conditions are true, throw (not invariant)
(1) an invalid NamespaceString is provided AND
(2) a dbVersion or shardVersion is

That way, commands that don't need to be versioned won't fail when they should pass. (See  DropConnectionsCmd for an example of a command that will always return an empty NamespaceString but shouldn't need to be versioned).

Comment by Kaloian Manassiev [ 25/Aug/20 ]

I don't quite understand the question. Are you saying that the namespace passed to the OSS is sometimes derived from how user formulated the command? Because if that is the case, yes we should uassert. If it however is derived from some internal code, then we have a bug and should fix it (therefore adding the invariant).

Comment by Haley Connelly [ 20/Aug/20 ]

Its not safe to invariant that the NSS is not empty inside the OperationShardingState.
Tests will hit that invariant when asserting commandFailed and fail later on.
eg) move_primary_basic.js

Despite not inheriting from InvocationBase, the listed commands still succeed and provide a valid namespace to the OSS when they aren't specifically designed to fail.

kaloian.manassiev We could uassert when the NamespaceString is empty rather than invariant. Or we could leave as is. Which do you prefer?

Comment by Haley Connelly [ 05/Jun/20 ]

Throughout OperationShardingState, we should probably enforce that the namespace passed in should not be empty (eg: for getShardVersion etc. as well).

Generated at Thu Feb 08 05:17:38 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.