Skip to content

Commit d2b1639

Browse files
JamesHeppenstallJamesHeppenstall
committed
SERVER-42329 Remove use of stringified id (concatenation of ns and minKey) in MigrationType
1 parent 12c28db commit d2b1639

File tree

8 files changed

+65
-68
lines changed

8 files changed

+65
-68
lines changed

‎src/mongo/db/s/balancer/balancer_policy.cpp‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -537,7 +537,24 @@ MigrateInfo::MigrateInfo(const ShardId& a_to, const ChunkType& a_chunk) {
537537
}
538538

539539
std::string MigrateInfo::getName() const {
540-
return MigrationType::genID(nss, minKey);
540+
// Generates a unique name for a MigrateInfo based on the namespace and the lower bound of the
541+
// chunk being moved.
542+
StringBuilder buf;
543+
buf << nss.ns() << "-";
544+
545+
BSONObjIterator i(minKey);
546+
while (i.more()) {
547+
BSONElement e = i.next();
548+
buf << e.fieldName() << "_" << e.toString(false, true);
549+
}
550+
551+
return buf.str();
552+
}
553+
554+
BSONObj MigrateInfo::getMigrationTypeQuery() const {
555+
// Generates a query object for a single MigrationType based on the namespace and the lower
556+
// bound of the chunk being moved.
557+
return BSON(MigrationType::ns(nss.ns()) << MigrationType::min(minKey));
541558
}
542559

543560
string MigrateInfo::toString() const {

‎src/mongo/db/s/balancer/balancer_policy.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ struct MigrateInfo {
5656

5757
std::string getName() const;
5858

59+
BSONObj getMigrationTypeQuery() const;
60+
5961
std::string toString() const;
6062

6163
NamespaceString nss;

‎src/mongo/db/s/balancer/migration_manager_test.cpp‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "mongo/db/commands.h"
3636
#include "mongo/db/s/balancer/migration_manager.h"
3737
#include "mongo/db/s/balancer/type_migration.h"
38+
#include "mongo/db/s/config/sharding_catalog_manager.h"
3839
#include "mongo/db/write_concern_options.h"
3940
#include "mongo/s/catalog/dist_lock_manager_mock.h"
4041
#include "mongo/s/catalog/type_collection.h"
@@ -161,6 +162,8 @@ class MigrationManagerTest : public ConfigServerTestFixture {
161162

162163
void MigrationManagerTest::setUp() {
163164
ConfigServerTestFixture::setUp();
165+
ASSERT_OK(ShardingCatalogManager::get(operationContext())
166+
->initializeConfigDatabaseIfNeeded(operationContext()));
164167
_migrationManager = std::make_unique<MigrationManager>(getServiceContext());
165168
_migrationManager->startRecoveryAndAcquireDistLocks(operationContext());
166169
_migrationManager->finishRecovery(operationContext(), 0, kDefaultSecondaryThrottle);
@@ -440,9 +443,9 @@ TEST_F(MigrationManagerTest, SourceShardNotFound) {
440443
MigrationStatuses migrationStatuses = _migrationManager->executeMigrationsForAutoBalance(
441444
opCtx.get(), migrationRequests, 0, kDefaultSecondaryThrottle, false);
442445

443-
ASSERT_OK(migrationStatuses.at(MigrationType::genID(chunk1.getNS(), chunk1.getMin())));
446+
ASSERT_OK(migrationStatuses.at(migrationRequests.front().getName()));
444447
ASSERT_EQ(ErrorCodes::ReplicaSetNotFound,
445-
migrationStatuses.at(MigrationType::genID(chunk2.getNS(), chunk2.getMin())));
448+
migrationStatuses.at(migrationRequests.back().getName()));
446449
});
447450

448451
// Expect only one moveChunk command to be called.
@@ -485,7 +488,7 @@ TEST_F(MigrationManagerTest, JumboChunkResponseBackwardsCompatibility) {
485488
opCtx.get(), migrationRequests, 0, kDefaultSecondaryThrottle, false);
486489

487490
ASSERT_EQ(ErrorCodes::ChunkTooBig,
488-
migrationStatuses.at(MigrationType::genID(chunk1.getNS(), chunk1.getMin())));
491+
migrationStatuses.at(migrationRequests.front().getName()));
489492
});
490493

491494
// Expect only one moveChunk command to be called.
@@ -560,7 +563,7 @@ TEST_F(MigrationManagerTest, InterruptMigration) {
560563
ReadPreferenceSetting{ReadPreference::PrimaryOnly},
561564
repl::ReadConcernLevel::kMajorityReadConcern,
562565
MigrationType::ConfigNS,
563-
BSON(MigrationType::name(MigrationType::genID(collName, chunk.getMin()))),
566+
BSON(MigrationType::ns(collName.ns()) << MigrationType::min(chunk.getMin())),
564567
BSONObj(),
565568
boost::none);
566569
Shard::QueryResponse migrationsQueryResponse =
@@ -570,7 +573,7 @@ TEST_F(MigrationManagerTest, InterruptMigration) {
570573
ASSERT_OK(catalogClient()->removeConfigDocuments(
571574
operationContext(),
572575
MigrationType::ConfigNS,
573-
BSON(MigrationType::name(MigrationType::genID(collName, chunk.getMin()))),
576+
BSON(MigrationType::ns(collName.ns()) << MigrationType::min(chunk.getMin())),
574577
kMajorityWriteConcern));
575578

576579
// Restore the migration manager back to the started state, which is expected by tearDown
@@ -756,6 +759,9 @@ TEST_F(MigrationManagerTest, RemoteCallErrorConversionToOperationFailed) {
756759
ChunkType chunk2 =
757760
setUpChunk(collName, BSON(kPattern << 49), kKeyPattern.globalMax(), kShardId2, version);
758761

762+
// Going to request that these two chunks get migrated.
763+
const std::vector<MigrateInfo> migrationRequests{{kShardId1, chunk1}, {kShardId3, chunk2}};
764+
759765
auto future = launchAsync([&] {
760766
ThreadClient tc("Test", getGlobalServiceContext());
761767
auto opCtx = cc().makeOperationContext();
@@ -766,16 +772,12 @@ TEST_F(MigrationManagerTest, RemoteCallErrorConversionToOperationFailed) {
766772
shardTargeterMock(opCtx.get(), kShardId2)->setFindHostReturnValue(kShardHost2);
767773

768774
MigrationStatuses migrationStatuses = _migrationManager->executeMigrationsForAutoBalance(
769-
opCtx.get(),
770-
{{kShardId1, chunk1}, {kShardId3, chunk2}},
771-
0,
772-
kDefaultSecondaryThrottle,
773-
false);
775+
opCtx.get(), migrationRequests, 0, kDefaultSecondaryThrottle, false);
774776

775777
ASSERT_EQ(ErrorCodes::OperationFailed,
776-
migrationStatuses.at(MigrationType::genID(collName, chunk1.getMin())));
778+
migrationStatuses.at(migrationRequests.front().getName()));
777779
ASSERT_EQ(ErrorCodes::OperationFailed,
778-
migrationStatuses.at(MigrationType::genID(collName, chunk2.getMin())));
780+
migrationStatuses.at(migrationRequests.back().getName()));
779781
});
780782

781783
// Expect a moveChunk command that will fail with a retriable error.

‎src/mongo/db/s/balancer/scoped_migration_request.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ StatusWith<ScopedMigrationRequest> ScopedMigrationRequest::writeMigration(
112112
ReadPreferenceSetting{ReadPreference::PrimaryOnly},
113113
repl::ReadConcernLevel::kLocalReadConcern,
114114
MigrationType::ConfigNS,
115-
BSON(MigrationType::name(migrateInfo.getName())),
115+
migrateInfo.getMigrationTypeQuery(),
116116
BSONObj(),
117117
boost::none);
118118
if (!statusWithMigrationQueryResult.isOK()) {

‎src/mongo/db/s/balancer/scoped_migration_request_test.cpp‎

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include "mongo/db/s/balancer/scoped_migration_request.h"
3333

3434
#include "mongo/db/s/balancer/type_migration.h"
35+
#include "mongo/db/s/config/sharding_catalog_manager.h"
3536
#include "mongo/s/client/shard_registry.h"
3637
#include "mongo/s/config_server_test_fixture.h"
3738
#include "mongo/s/request_types/migration_secondary_throttle_options.h"
@@ -51,10 +52,10 @@ const ShardId kDifferentToShard("shard0002");
5152
class ScopedMigrationRequestTest : public ConfigServerTestFixture {
5253
public:
5354
/**
54-
* Queries config.migrations for a document with name (_id) "chunkName" and asserts that the
55+
* Queries config.migrations for the document pertaining to migrateInfo and asserts that the
5556
* number of documents returned equals "expectedNumberOfDocuments".
5657
*/
57-
void checkMigrationsCollectionForDocument(std::string chunkName,
58+
void checkMigrationsCollectionForDocument(const MigrateInfo& migrateInfo,
5859
const unsigned long expectedNumberOfDocuments);
5960

6061
/**
@@ -63,16 +64,25 @@ class ScopedMigrationRequestTest : public ConfigServerTestFixture {
6364
* constructors.
6465
*/
6566
ScopedMigrationRequest makeScopedMigrationRequest(const MigrateInfo& migrateInfo);
67+
68+
private:
69+
void setUp() override;
6670
};
6771

72+
void ScopedMigrationRequestTest::setUp() {
73+
ConfigServerTestFixture::setUp();
74+
ASSERT_OK(ShardingCatalogManager::get(operationContext())
75+
->initializeConfigDatabaseIfNeeded(operationContext()));
76+
}
77+
6878
void ScopedMigrationRequestTest::checkMigrationsCollectionForDocument(
69-
std::string chunkName, const unsigned long expectedNumberOfDocuments) {
79+
const MigrateInfo& migrateInfo, const unsigned long expectedNumberOfDocuments) {
7080
auto response = shardRegistry()->getConfigShard()->exhaustiveFindOnConfig(
7181
operationContext(),
7282
ReadPreferenceSetting{ReadPreference::PrimaryOnly},
7383
repl::ReadConcernLevel::kMajorityReadConcern,
7484
MigrationType::ConfigNS,
75-
BSON(MigrationType::name(chunkName)),
85+
migrateInfo.getMigrationTypeQuery(),
7686
BSONObj(),
7787
boost::none);
7888
Shard::QueryResponse queryResponse = unittest::assertGet(response);
@@ -85,7 +95,7 @@ ScopedMigrationRequest ScopedMigrationRequestTest::makeScopedMigrationRequest(
8595
ScopedMigrationRequest scopedMigrationRequest =
8696
assertGet(ScopedMigrationRequest::writeMigration(operationContext(), migrateInfo, false));
8797

88-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
98+
checkMigrationsCollectionForDocument(migrateInfo, 1);
8999

90100
return scopedMigrationRequest;
91101
}
@@ -113,10 +123,10 @@ TEST_F(ScopedMigrationRequestTest, CreateScopedMigrationRequest) {
113123
ScopedMigrationRequest scopedMigrationRequest = assertGet(
114124
ScopedMigrationRequest::writeMigration(operationContext(), migrateInfo, false));
115125

116-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
126+
checkMigrationsCollectionForDocument(migrateInfo, 1);
117127
}
118128

119-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 0);
129+
checkMigrationsCollectionForDocument(migrateInfo, 0);
120130
}
121131

122132
/**
@@ -134,12 +144,12 @@ TEST_F(ScopedMigrationRequestTest, CreateScopedMigrationRequestOnRecovery) {
134144
ScopedMigrationRequest scopedMigrationRequest = assertGet(
135145
ScopedMigrationRequest::writeMigration(operationContext(), migrateInfo, false));
136146

137-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
147+
checkMigrationsCollectionForDocument(migrateInfo, 1);
138148

139149
scopedMigrationRequest.keepDocumentOnDestruct();
140150
}
141151

142-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
152+
checkMigrationsCollectionForDocument(migrateInfo, 1);
143153

144154
// Fail to write a migration document if a migration document already exists for that chunk but
145155
// with a different destination shard. (the migration request must have identical parameters).
@@ -153,7 +163,7 @@ TEST_F(ScopedMigrationRequestTest, CreateScopedMigrationRequestOnRecovery) {
153163

154164
ASSERT_EQUALS(ErrorCodes::DuplicateKey, statusWithScopedMigrationRequest.getStatus());
155165

156-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
166+
checkMigrationsCollectionForDocument(migrateInfo, 1);
157167
}
158168

159169
// Create a new scoped object without inserting a document, and check that the destructor
@@ -162,10 +172,10 @@ TEST_F(ScopedMigrationRequestTest, CreateScopedMigrationRequestOnRecovery) {
162172
ScopedMigrationRequest scopedMigrationRequest = ScopedMigrationRequest::createForRecovery(
163173
operationContext(), migrateInfo.nss, migrateInfo.minKey);
164174

165-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
175+
checkMigrationsCollectionForDocument(migrateInfo, 1);
166176
}
167177

168-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 0);
178+
checkMigrationsCollectionForDocument(migrateInfo, 0);
169179
}
170180

171181
TEST_F(ScopedMigrationRequestTest, CreateMultipleScopedMigrationRequestsForIdenticalMigration) {
@@ -176,22 +186,22 @@ TEST_F(ScopedMigrationRequestTest, CreateMultipleScopedMigrationRequestsForIdent
176186
ScopedMigrationRequest scopedMigrationRequest = assertGet(
177187
ScopedMigrationRequest::writeMigration(operationContext(), migrateInfo, false));
178188

179-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
189+
checkMigrationsCollectionForDocument(migrateInfo, 1);
180190

181191
{
182192
// Should be able to create another Scoped object if the request is identical.
183193
ScopedMigrationRequest identicalScopedMigrationRequest = assertGet(
184194
ScopedMigrationRequest::writeMigration(operationContext(), migrateInfo, false));
185195

186-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
196+
checkMigrationsCollectionForDocument(migrateInfo, 1);
187197
}
188198

189199
// If any scoped object goes out of scope, the migration should be over and the document
190200
// removed.
191-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 0);
201+
checkMigrationsCollectionForDocument(migrateInfo, 0);
192202
}
193203

194-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 0);
204+
checkMigrationsCollectionForDocument(migrateInfo, 0);
195205
}
196206

197207
TEST_F(ScopedMigrationRequestTest, TryToRemoveScopedMigrationRequestBeforeDestruct) {
@@ -201,11 +211,11 @@ TEST_F(ScopedMigrationRequestTest, TryToRemoveScopedMigrationRequestBeforeDestru
201211
ScopedMigrationRequest scopedMigrationRequest =
202212
assertGet(ScopedMigrationRequest::writeMigration(operationContext(), migrateInfo, false));
203213

204-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
214+
checkMigrationsCollectionForDocument(migrateInfo, 1);
205215

206216
ASSERT_OK(scopedMigrationRequest.tryToRemoveMigration());
207217

208-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 0);
218+
checkMigrationsCollectionForDocument(migrateInfo, 0);
209219
}
210220

211221
TEST_F(ScopedMigrationRequestTest, MoveAndAssignmentConstructors) {
@@ -217,10 +227,10 @@ TEST_F(ScopedMigrationRequestTest, MoveAndAssignmentConstructors) {
217227
ScopedMigrationRequest anotherScopedMigrationRequest =
218228
makeScopedMigrationRequest(migrateInfo);
219229

220-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 1);
230+
checkMigrationsCollectionForDocument(migrateInfo, 1);
221231
}
222232

223-
checkMigrationsCollectionForDocument(migrateInfo.getName(), 0);
233+
checkMigrationsCollectionForDocument(migrateInfo, 0);
224234
}
225235

226236
} // namespace

‎src/mongo/db/s/balancer/type_migration.cpp‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ const StringData kChunkVersion = "chunkVersion"_sd;
4444

4545
const NamespaceString MigrationType::ConfigNS("config.migrations");
4646

47-
const BSONField<std::string> MigrationType::name("_id");
4847
const BSONField<std::string> MigrationType::ns("ns");
4948
const BSONField<BSONObj> MigrationType::min("min");
5049
const BSONField<BSONObj> MigrationType::max("max");
@@ -122,7 +121,6 @@ StatusWith<MigrationType> MigrationType::fromBSON(const BSONObj& source) {
122121
BSONObj MigrationType::toBSON() const {
123122
BSONObjBuilder builder;
124123

125-
builder.append(name.name(), getName());
126124
builder.append(ns.name(), _nss.ns());
127125

128126
builder.append(min.name(), _min);
@@ -148,21 +146,4 @@ MigrateInfo MigrationType::toMigrateInfo() const {
148146
return MigrateInfo(_toShard, chunk);
149147
}
150148

151-
std::string MigrationType::getName() const {
152-
return genID(_nss, _min);
153-
}
154-
155-
std::string MigrationType::genID(const NamespaceString& nss, const BSONObj& o) {
156-
StringBuilder buf;
157-
buf << nss.ns() << "-";
158-
159-
BSONObjIterator i(o);
160-
while (i.more()) {
161-
BSONElement e = i.next();
162-
buf << e.fieldName() << "_" << e.toString(false, true);
163-
}
164-
165-
return buf.str();
166-
}
167-
168149
} // namespace mongo

‎src/mongo/db/s/balancer/type_migration.h‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ class MigrationType {
4848
static const NamespaceString ConfigNS;
4949

5050
// Field names and types in the migrations collection type.
51-
static const BSONField<std::string> name;
5251
static const BSONField<std::string> ns;
5352
static const BSONField<BSONObj> min;
5453
static const BSONField<BSONObj> max;
@@ -78,16 +77,6 @@ class MigrationType {
7877
*/
7978
MigrateInfo toMigrateInfo() const;
8079

81-
/**
82-
* Uniquely identifies a chunk by collection and min key.
83-
*/
84-
std::string getName() const;
85-
86-
/**
87-
* Generates id based on the namespace and the lower bound of the chunk being moved.
88-
*/
89-
static std::string genID(const NamespaceString& nss, const BSONObj& o);
90-
9180
const NamespaceString& getNss() const {
9281
return _nss;
9382
}

‎src/mongo/db/s/balancer/type_migration_test.cpp‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ namespace {
4040

4141
using unittest::assertGet;
4242

43-
const std::string kName = "TestDB.TestColl-a_10";
4443
const std::string kNs = "TestDB.TestColl";
4544
const BSONObj kMin = BSON("a" << 10);
4645
const BSONObj kMax = BSON("a" << 20);
@@ -66,7 +65,6 @@ TEST(MigrationTypeTest, ConvertFromMigrationInfo) {
6665
MigrationType migrationType(migrateInfo, kWaitForDelete);
6766

6867
BSONObjBuilder builder;
69-
builder.append(MigrationType::name(), kName);
7068
builder.append(MigrationType::ns(), kNs);
7169
builder.append(MigrationType::min(), kMin);
7270
builder.append(MigrationType::max(), kMax);
@@ -84,7 +82,6 @@ TEST(MigrationTypeTest, FromAndToBSON) {
8482
const ChunkVersion version(1, 2, OID::gen());
8583

8684
BSONObjBuilder builder;
87-
builder.append(MigrationType::name(), kName);
8885
builder.append(MigrationType::ns(), kNs);
8986
builder.append(MigrationType::min(), kMin);
9087
builder.append(MigrationType::max(), kMax);
@@ -186,7 +183,6 @@ TEST(MigrationTypeTest, MissingRequiredToShardField) {
186183

187184
TEST(MigrationTypeTest, MissingRequiredVersionField) {
188185
BSONObjBuilder builder;
189-
builder.append(MigrationType::name(), kName);
190186
builder.append(MigrationType::ns(), kNs);
191187
builder.append(MigrationType::min(), kMin);
192188
builder.append(MigrationType::max(), kMax);

0 commit comments

Comments
 (0)