Skip to content

Commit 12ac469

Browse files
benetybenety
committed
SERVER-29274 make reaper mandatory in Database::dropCollection()
1 parent a0ddbc7 commit 12ac469

File tree

5 files changed

+99
-121
lines changed

5 files changed

+99
-121
lines changed

‎src/mongo/db/catalog/database_impl.cpp‎

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -448,27 +448,23 @@ Status DatabaseImpl::dropCollectionEvenIfSystem(OperationContext* opCtx,
448448
auto uuid = collection->uuid();
449449

450450
// Drop unreplicated collections immediately.
451-
// Replicated collections should also be dropped immediately if there is no active reaper to
452-
// remove the drop-pending collections.
453-
// If 'dropOpTime' is provided and the reaper is available, we should proceed to rename the
454-
// collection.
455-
auto isOplogDisabledForNamespace =
456-
repl::ReplicationCoordinator::get(opCtx)->isOplogDisabledFor(opCtx, fullns);
457-
auto reaper = repl::DropPendingCollectionReaper::get(opCtx);
458-
if ((dropOpTime.isNull() && isOplogDisabledForNamespace) || !reaper) {
451+
// If 'dropOpTime' is provided, we should proceed to rename the collection.
452+
auto replCoord = repl::ReplicationCoordinator::get(opCtx);
453+
auto opObserver = getGlobalServiceContext()->getOpObserver();
454+
auto isOplogDisabledForNamespace = replCoord->isOplogDisabledFor(opCtx, fullns);
455+
if (dropOpTime.isNull() && isOplogDisabledForNamespace) {
459456
auto status = _finishDropCollection(opCtx, fullns, collection);
460457
if (!status.isOK()) {
461458
return status;
462459
}
463-
getGlobalServiceContext()->getOpObserver()->onDropCollection(opCtx, fullns, uuid);
460+
opObserver->onDropCollection(opCtx, fullns, uuid);
464461
return Status::OK();
465462
}
466463

467464
// Replicated collections will be renamed with a special drop-pending namespace and dropped when
468465
// the replica set optime reaches the drop optime.
469466
if (dropOpTime.isNull()) {
470-
dropOpTime =
471-
getGlobalServiceContext()->getOpObserver()->onDropCollection(opCtx, fullns, uuid);
467+
dropOpTime = opObserver->onDropCollection(opCtx, fullns, uuid);
472468

473469
// Drop collection immediately if OpObserver did not write entry to oplog.
474470
// After writing the oplog entry, all errors are fatal. See getNextOpTime() comments in
@@ -485,8 +481,7 @@ Status DatabaseImpl::dropCollectionEvenIfSystem(OperationContext* opCtx,
485481
// in the context of applying an oplog entry on a secondary.
486482
// OpObserver::onDropCollection() should be returning a null OpTime because we should not be
487483
// writing to the oplog.
488-
auto opTime =
489-
getGlobalServiceContext()->getOpObserver()->onDropCollection(opCtx, fullns, uuid);
484+
auto opTime = opObserver->onDropCollection(opCtx, fullns, uuid);
490485
if (!opTime.isNull()) {
491486
severe() << "dropCollection: " << fullns
492487
<< " - unexpected oplog entry written to the oplog with optime " << opTime;
@@ -514,8 +509,7 @@ Status DatabaseImpl::dropCollectionEvenIfSystem(OperationContext* opCtx,
514509

515510
// Register this drop-pending namespace with DropPendingCollectionReaper to remove when the
516511
// committed optime reaches the drop optime.
517-
invariant(reaper);
518-
reaper->addDropPendingNamespace(dropOpTime, dpns);
512+
repl::DropPendingCollectionReaper::get(opCtx)->addDropPendingNamespace(dropOpTime, dpns);
519513

520514
return Status::OK();
521515
}

‎src/mongo/db/catalog/database_test.cpp‎

Lines changed: 9 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,11 @@ void DatabaseTest::setUp() {
7474
auto service = getServiceContext();
7575
_opCtx = cc().makeOperationContext();
7676

77+
repl::StorageInterface::set(service, stdx::make_unique<repl::StorageInterfaceMock>());
78+
repl::DropPendingCollectionReaper::set(
79+
service,
80+
stdx::make_unique<repl::DropPendingCollectionReaper>(repl::StorageInterface::get(service)));
81+
7782
// Set up ReplicationCoordinator and create oplog.
7883
repl::ReplicationCoordinator::set(service,
7984
stdx::make_unique<repl::ReplicationCoordinatorMock>(service));
@@ -145,7 +150,7 @@ TEST_F(DatabaseTest, DropCollectionDropsCollectionButDoesNotLogOperationIfWrites
145150
}
146151

147152
TEST_F(DatabaseTest,
148-
DropCollectionDropsCollectionAndLogsOperationIfWritesAreReplicatedButReaperIsNotAvailable) {
153+
DropCollectionRenamesCollectionToPendingDropNamespaceAndLogsOperationIfWritesAreReplicated) {
149154
ASSERT_TRUE(_opCtx->writesAreReplicated());
150155
ASSERT_FALSE(
151156
repl::ReplicationCoordinator::get(_opCtx.get())->isOplogDisabledFor(_opCtx.get(), _nss));
@@ -156,39 +161,14 @@ TEST_F(DatabaseTest,
156161
auto dropOpTime = repl::ReplClientInfo::forClient(&cc()).getLastOp();
157162
ASSERT_GREATER_THAN(dropOpTime, repl::OpTime());
158163

159-
// If the drop-pending collection reaper is not available, the collection is not renamed to
160-
// <db>.system.drop.*.
161-
auto dpns = _nss.makeDropPendingNamespace(dropOpTime);
162-
ASSERT_FALSE(mongo::AutoGetCollectionForRead(_opCtx.get(), dpns).getCollection());
163-
}
164-
165-
TEST_F(
166-
DatabaseTest,
167-
DropCollectionRenamesCollectionToPendingDropNamespaceAndLogsOperationIfWritesAreReplicatedAndReaperIsAvailable) {
168-
ASSERT_TRUE(_opCtx->writesAreReplicated());
169-
ASSERT_FALSE(
170-
repl::ReplicationCoordinator::get(_opCtx.get())->isOplogDisabledFor(_opCtx.get(), _nss));
171-
172-
auto service = getServiceContext();
173-
repl::StorageInterface::set(service, stdx::make_unique<repl::StorageInterfaceMock>());
174-
repl::DropPendingCollectionReaper::set(
175-
service,
176-
stdx::make_unique<repl::DropPendingCollectionReaper>(repl::StorageInterface::get(service)));
177-
178-
_testDropCollection(_opCtx.get(), _nss, true);
179-
180-
// Drop optime is non-null because an op was written to the oplog.
181-
auto dropOpTime = repl::ReplClientInfo::forClient(&cc()).getLastOp();
182-
ASSERT_GREATER_THAN(dropOpTime, repl::OpTime());
183-
184164
// Replicated collection is renamed with a special drop-pending names in the <db>.system.drop.*
185165
// namespace.
186166
auto dpns = _nss.makeDropPendingNamespace(dropOpTime);
187167
ASSERT_TRUE(mongo::AutoGetCollectionForRead(_opCtx.get(), dpns).getCollection());
188168

189169
// Reaper should have the drop optime of the collection.
190170
auto reaperEarliestDropOpTime =
191-
repl::DropPendingCollectionReaper::get(service)->getEarliestDropOpTime();
171+
repl::DropPendingCollectionReaper::get(_opCtx.get())->getEarliestDropOpTime();
192172
ASSERT_TRUE(reaperEarliestDropOpTime);
193173
ASSERT_EQUALS(dropOpTime, *reaperEarliestDropOpTime);
194174
}
@@ -213,39 +193,14 @@ TEST_F(DatabaseTest, DropCollectionRejectsProvidedDropOpTimeIfWritesAreReplicate
213193
});
214194
}
215195

216-
TEST_F(DatabaseTest,
217-
DropCollectionIgnoresProvidedDropOpTimeAndDropsCollectionImmediatelyIfReaperIsNotAvailable) {
218-
repl::UnreplicatedWritesBlock uwb(_opCtx.get());
219-
ASSERT_FALSE(_opCtx->writesAreReplicated());
220-
ASSERT_TRUE(
221-
repl::ReplicationCoordinator::get(_opCtx.get())->isOplogDisabledFor(_opCtx.get(), _nss));
222-
223-
repl::OpTime dropOpTime(Timestamp(Seconds(100), 0), 1LL);
224-
_testDropCollection(_opCtx.get(), _nss, true, dropOpTime);
225-
226-
// Last optime in repl client is null because we did not write to the oplog.
227-
ASSERT_EQUALS(repl::OpTime(), repl::ReplClientInfo::forClient(&cc()).getLastOp());
228-
229-
// If the drop-pending collection reaper is not available, the collection is not renamed to
230-
// <db>.system.drop.*.
231-
auto dpns = _nss.makeDropPendingNamespace(dropOpTime);
232-
ASSERT_FALSE(mongo::AutoGetCollectionForRead(_opCtx.get(), dpns).getCollection());
233-
}
234-
235196
TEST_F(
236197
DatabaseTest,
237-
DropCollectionRenamesCollectionToPendingDropNamespaceUsingProvidedDropOpTimeButDoesNotLogOperationIfReaperIsAvailable) {
198+
DropCollectionRenamesCollectionToPendingDropNamespaceUsingProvidedDropOpTimeButDoesNotLogOperation) {
238199
repl::UnreplicatedWritesBlock uwb(_opCtx.get());
239200
ASSERT_FALSE(_opCtx->writesAreReplicated());
240201
ASSERT_TRUE(
241202
repl::ReplicationCoordinator::get(_opCtx.get())->isOplogDisabledFor(_opCtx.get(), _nss));
242203

243-
auto service = getServiceContext();
244-
repl::StorageInterface::set(service, stdx::make_unique<repl::StorageInterfaceMock>());
245-
repl::DropPendingCollectionReaper::set(
246-
service,
247-
stdx::make_unique<repl::DropPendingCollectionReaper>(repl::StorageInterface::get(service)));
248-
249204
repl::OpTime dropOpTime(Timestamp(Seconds(100), 0), 1LL);
250205
_testDropCollection(_opCtx.get(), _nss, true, dropOpTime);
251206

@@ -259,7 +214,7 @@ TEST_F(
259214

260215
// Reaper should have the drop optime of the collection.
261216
auto reaperEarliestDropOpTime =
262-
repl::DropPendingCollectionReaper::get(service)->getEarliestDropOpTime();
217+
repl::DropPendingCollectionReaper::get(_opCtx.get())->getEarliestDropOpTime();
263218
ASSERT_TRUE(reaperEarliestDropOpTime);
264219
ASSERT_EQUALS(dropOpTime, *reaperEarliestDropOpTime);
265220
}

‎src/mongo/db/repl/sync_tail_test.cpp‎

Lines changed: 76 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
#include "mongo/db/jsobj.h"
4747
#include "mongo/db/query/internal_plans.h"
4848
#include "mongo/db/repl/bgsync.h"
49+
#include "mongo/db/repl/drop_pending_collection_reaper.h"
4950
#include "mongo/db/repl/oplog.h"
5051
#include "mongo/db/repl/oplog_interface_local.h"
5152
#include "mongo/db/repl/replication_consistency_markers_mock.h"
@@ -120,6 +121,8 @@ void SyncTailTest::setUp() {
120121
const NamespaceString&,
121122
const std::vector<BSONObj>&) { return Status::OK(); };
122123
StorageInterface::set(service, std::move(storageInterface));
124+
DropPendingCollectionReaper::set(
125+
service, stdx::make_unique<DropPendingCollectionReaper>(_storageInterface));
123126

124127
_replicationProcess = new ReplicationProcess(
125128
_storageInterface, stdx::make_unique<ReplicationConsistencyMarkersMock>());
@@ -139,10 +142,12 @@ void SyncTailTest::setUp() {
139142
}
140143

141144
void SyncTailTest::tearDown() {
145+
auto service = getServiceContext();
142146
_opCtx.reset();
147+
ReplicationProcess::set(service, {});
148+
DropPendingCollectionReaper::set(service, {});
149+
StorageInterface::set(service, {});
143150
ServiceContextMongoDTest::tearDown();
144-
_storageInterface = nullptr;
145-
ReplicationProcess::set(getServiceContext(), {});
146151
}
147152

148153
SyncTailWithLocalDocumentFetcher::SyncTailWithLocalDocumentFetcher(const BSONObj& document)
@@ -1315,57 +1320,70 @@ TEST_F(IdempotencyTest, TextIndexDocumentHasUnknownLanguage) {
13151320
}
13161321

13171322
TEST_F(IdempotencyTest, CreateCollectionWithValidation) {
1318-
getGlobalReplicationCoordinator()->setFollowerMode(MemberState::RS_RECOVERING);
1319-
auto options1 = fromjson("{'validator' : {'phone' : {'$type' : 'string' } } }");
1320-
auto createColl1 = makeCreateCollectionOplogEntry(nextOpTime(), nss, options1);
1321-
auto dropColl = makeCommandOplogEntry(nextOpTime(), nss, BSON("drop" << nss.coll()));
1322-
auto options2 = fromjson("{'validator' : {'phone' : {'$type' : 'number' } } }");
1323-
auto createColl2 = makeCreateCollectionOplogEntry(nextOpTime(), nss, options2);
1323+
ReplicationCoordinator::get(_opCtx.get())->setFollowerMode(MemberState::RS_RECOVERING);
13241324

1325-
auto ops = {createColl1, dropColl, createColl2};
1325+
auto runOpsAndValidate = [this]() {
1326+
auto options1 = fromjson("{'validator' : {'phone' : {'$type' : 'string' } } }");
1327+
auto createColl1 = makeCreateCollectionOplogEntry(nextOpTime(), nss, options1);
1328+
auto dropColl = makeCommandOplogEntry(nextOpTime(), nss, BSON("drop" << nss.coll()));
1329+
auto options2 = fromjson("{'validator' : {'phone' : {'$type' : 'number' } } }");
1330+
auto createColl2 = makeCreateCollectionOplogEntry(nextOpTime(), nss, options2);
13261331

1327-
ASSERT_OK(runOps(ops));
1328-
auto hash = validate();
1329-
ASSERT_OK(runOps(ops));
1330-
ASSERT_EQUALS(hash, validate());
1332+
auto ops = {createColl1, dropColl, createColl2};
1333+
1334+
ASSERT_OK(runOps(ops));
1335+
auto hash = validate();
1336+
1337+
return hash;
1338+
};
1339+
1340+
auto hash1 = runOpsAndValidate();
1341+
auto hash2 = runOpsAndValidate();
1342+
ASSERT_EQUALS(hash1, hash2);
13311343
}
13321344

13331345
TEST_F(IdempotencyTest, CreateCollectionWithCollation) {
13341346
getGlobalReplicationCoordinator()->setFollowerMode(MemberState::RS_RECOVERING);
13351347
ASSERT_OK(runOp(createCollection()));
13361348

1337-
auto insertOp1 = insert(fromjson("{ _id: 'foo' }"));
1338-
auto insertOp2 = insert(fromjson("{ _id: 'Foo', x: 1 }"));
1339-
auto updateOp = update("foo", BSON("$set" << BSON("x" << 2)));
1340-
auto dropColl = makeCommandOplogEntry(nextOpTime(), nss, BSON("drop" << nss.coll()));
1341-
auto options = BSON("collation" << BSON("locale"
1342-
<< "en"
1343-
<< "caseLevel"
1344-
<< false
1345-
<< "caseFirst"
1346-
<< "off"
1347-
<< "strength"
1348-
<< 1
1349-
<< "numericOrdering"
1350-
<< false
1351-
<< "alternate"
1352-
<< "non-ignorable"
1353-
<< "maxVariable"
1354-
<< "punct"
1355-
<< "normalization"
1356-
<< false
1357-
<< "backwards"
1358-
<< false
1359-
<< "version"
1360-
<< "57.1"));
1361-
auto createColl = makeCreateCollectionOplogEntry(nextOpTime(), nss, options);
1362-
1363-
auto ops = {insertOp1, insertOp2, updateOp, dropColl, createColl};
1349+
auto runOpsAndValidate = [this]() {
1350+
auto insertOp1 = insert(fromjson("{ _id: 'foo' }"));
1351+
auto insertOp2 = insert(fromjson("{ _id: 'Foo', x: 1 }"));
1352+
auto updateOp = update("foo", BSON("$set" << BSON("x" << 2)));
1353+
auto dropColl = makeCommandOplogEntry(nextOpTime(), nss, BSON("drop" << nss.coll()));
1354+
auto options = BSON("collation" << BSON("locale"
1355+
<< "en"
1356+
<< "caseLevel"
1357+
<< false
1358+
<< "caseFirst"
1359+
<< "off"
1360+
<< "strength"
1361+
<< 1
1362+
<< "numericOrdering"
1363+
<< false
1364+
<< "alternate"
1365+
<< "non-ignorable"
1366+
<< "maxVariable"
1367+
<< "punct"
1368+
<< "normalization"
1369+
<< false
1370+
<< "backwards"
1371+
<< false
1372+
<< "version"
1373+
<< "57.1"));
1374+
auto createColl = makeCreateCollectionOplogEntry(nextOpTime(), nss, options);
1375+
1376+
auto ops = {insertOp1, insertOp2, updateOp, dropColl, createColl};
1377+
1378+
ASSERT_OK(runOps(ops));
1379+
auto hash = validate();
1380+
1381+
return hash;
1382+
};
13641383

1365-
ASSERT_OK(runOps(ops));
1366-
auto hash = validate();
1367-
ASSERT_OK(runOps(ops));
1368-
ASSERT_EQUALS(hash, validate());
1384+
auto hash1 = runOpsAndValidate();
1385+
auto hash2 = runOpsAndValidate();
1386+
ASSERT_EQUALS(hash1, hash2);
13691387
}
13701388

13711389
TEST_F(IdempotencyTest, CreateCollectionWithIdIndex) {
@@ -1380,16 +1398,22 @@ TEST_F(IdempotencyTest, CreateCollectionWithIdIndex) {
13801398
auto createColl1 = makeCreateCollectionOplogEntry(nextOpTime(), nss, options1);
13811399
ASSERT_OK(runOp(createColl1));
13821400

1383-
auto insertOp = insert(BSON("_id" << Decimal128(1)));
1384-
auto dropColl = makeCommandOplogEntry(nextOpTime(), nss, BSON("drop" << nss.coll()));
1385-
auto createColl2 = createCollection();
1401+
auto runOpsAndValidate = [this]() {
1402+
auto insertOp = insert(BSON("_id" << Decimal128(1)));
1403+
auto dropColl = makeCommandOplogEntry(nextOpTime(), nss, BSON("drop" << nss.coll()));
1404+
auto createColl2 = createCollection();
13861405

1387-
auto ops = {insertOp, dropColl, createColl2};
1406+
auto ops = {insertOp, dropColl, createColl2};
13881407

1389-
ASSERT_OK(runOps(ops));
1390-
auto hash = validate();
1391-
ASSERT_OK(runOps(ops));
1392-
ASSERT_EQUALS(hash, validate());
1408+
ASSERT_OK(runOps(ops));
1409+
auto hash = validate();
1410+
1411+
return hash;
1412+
};
1413+
1414+
auto hash1 = runOpsAndValidate();
1415+
auto hash2 = runOpsAndValidate();
1416+
ASSERT_EQUALS(hash1, hash2);
13931417
}
13941418

13951419
TEST_F(IdempotencyTest, CreateCollectionWithView) {

‎src/mongo/s/SConscript‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ env.Library(
150150
LIBDEPS=[
151151
'$BUILD_DIR/mongo/client/remote_command_targeter_mock',
152152
'$BUILD_DIR/mongo/db/op_observer_d',
153+
'$BUILD_DIR/mongo/db/repl/drop_pending_collection_reaper',
153154
'$BUILD_DIR/mongo/db/repl/replmocks',
154155
'$BUILD_DIR/mongo/db/service_context_d_test_fixture',
155156
'$BUILD_DIR/mongo/executor/network_test_env',

‎src/mongo/s/sharding_mongod_test_fixture.cpp‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
#include "mongo/db/op_observer_impl.h"
4444
#include "mongo/db/query/cursor_response.h"
4545
#include "mongo/db/query/query_request.h"
46+
#include "mongo/db/repl/drop_pending_collection_reaper.h"
4647
#include "mongo/db/repl/oplog.h"
4748
#include "mongo/db/repl/read_concern_args.h"
4849
#include "mongo/db/repl/repl_settings.h"
@@ -128,6 +129,9 @@ void ShardingMongodTestFixture::setUp() {
128129

129130
auto storagePtr = stdx::make_unique<repl::StorageInterfaceMock>();
130131

132+
repl::DropPendingCollectionReaper::set(
133+
service, stdx::make_unique<repl::DropPendingCollectionReaper>(storagePtr.get()));
134+
131135
repl::ReplicationProcess::set(
132136
service,
133137
stdx::make_unique<repl::ReplicationProcess>(

0 commit comments

Comments
 (0)