SERVER-39141 Handle WriteConflictException in range deleter

3 files changed, 60 insertions, 10 deletions

diff --git a/src/mongo/db/s/collection_range_deleter.cpp b/src/mongo/db/s/collection_range_deleter.cpp
index e1bd537700c..1b63c1ce74c 100644
--- a/src/mongo/db/s/collection_range_deleter.cpp
+++ b/src/mongo/db/s/collection_range_deleter.cpp
@@ -112,7 +112,7 @@ boost::optional<Date_t> CollectionRangeDeleter::cleanUpNextRange(
         }
     }
 
-    StatusWith<int> wrote = 0;
+    StatusWith<int> swNumDeleted = 0;
 
     auto range = boost::optional<ChunkRange>(boost::none);
     auto notification = DeleteNotification();
@@ -201,22 +201,31 @@ boost::optional<Date_t> CollectionRangeDeleter::cleanUpNextRange(
         const auto& metadata = *scopedCollectionMetadata;
 
         try {
-            wrote = self->_doDeletion(
+            swNumDeleted = self->_doDeletion(
                 opCtx, collection, metadata->getKeyPattern(), *range, maxToDelete);
         } catch (const DBException& e) {
-            wrote = e.toStatus();
+            swNumDeleted = e.toStatus();
             warning() << e.what();
         }
     }  // drop autoColl
 
-    if (!wrote.isOK() || wrote.getValue() == 0) {
-        if (wrote.isOK()) {
+    bool continueDeleting = swNumDeleted.isOK() && swNumDeleted.getValue() > 0;
+
+    if (swNumDeleted == ErrorCodes::WriteConflict) {
+        CurOp::get(opCtx)->debug().additiveMetrics.incrementWriteConflicts(1);
+        continueDeleting = true;
+    }
+
+    // If there's an error or if there are no more documents to delete, take this branch.
+    // This branch means that we will NOT continue deleting documents from this range.
+    if (!continueDeleting) {
+        if (swNumDeleted.isOK()) {
             LOG(0) << "No documents remain to delete in " << nss << " range "
                    << redact(range->toString());
         }
 
-        // Wait for majority replication even when wrote isn't OK or == 0, because it might have
-        // been OK and/or > 0 previously, and the deletions must be persistent before notifying
+        // Wait for majority replication even when swNumDeleted isn't OK or == 0, because it might
+        // have been OK and/or > 0 previously, and the deletions must be persistent before notifying
         // clients in _pop().
 
         LOG(0) << "Waiting for majority replication of local deletions in " << nss.ns() << " range "
@@ -270,7 +279,7 @@ boost::optional<Date_t> CollectionRangeDeleter::cleanUpNextRange(
             LOG(0) << "Finished deleting documents in " << nss.ns() << " range "
                    << redact(range->toString());
 
-            self->_pop(wrote.getStatus());
+            self->_pop(swNumDeleted.getStatus());
         }
 
         if (!self->_orphans.empty()) {
@@ -282,8 +291,7 @@ boost::optional<Date_t> CollectionRangeDeleter::cleanUpNextRange(
     }
 
     invariant(range);
-    invariant(wrote.getStatus());
-    invariant(wrote.getValue() > 0);
+    invariant(continueDeleting);
 
     notification.abandon();
     return Date_t::now() + Milliseconds(rangeDeleterBatchDelayMS.load());
@@ -403,6 +411,11 @@ StatusWith<int> CollectionRangeDeleter::_doDeletion(OperationContext* opCtx,
     int numDeleted = 0;
     do {
         BSONObj deletedObj;
+
+        // TODO SERVER-41606: Remove this function when we refactor CollectionRangeDeleter.
+        if (_throwWriteConflictForTest)
+            throw WriteConflictException();
+
         PlanExecutor::ExecState state = exec->getNext(&deletedObj, nullptr);
 
         if (state == PlanExecutor::IS_EOF) {
diff --git a/src/mongo/db/s/collection_range_deleter.h b/src/mongo/db/s/collection_range_deleter.h
index 9de86f4c694..6fae0ee5d18 100644
--- a/src/mongo/db/s/collection_range_deleter.h
+++ b/src/mongo/db/s/collection_range_deleter.h
@@ -183,6 +183,11 @@ public:
                                                     int maxToDelete = 0,
                                                     CollectionRangeDeleter* forTestOnly = nullptr);
 
+    // TODO SERVER-41606: Remove this function when we refactor CollectionRangeDeleter.
+    void setDoDeletionShouldThrowWriteConflictForTest(bool on) {
+        _throwWriteConflictForTest = on;
+    }
+
 private:
     /**
      * Verifies that the metadata for the collection to be cleaned up is still valid. Makes sure
@@ -214,6 +219,9 @@ private:
      */
     void _pop(Status status);
 
+    // TODO SERVER-41606: Remove this function when we refactor CollectionRangeDeleter.
+    bool _throwWriteConflictForTest{false};
+
     /**
      * Ranges scheduled for deletion.  The front of the list will be in active process of deletion.
      * As each range is completed, its notification is signaled before it is popped.
diff --git a/src/mongo/db/s/collection_range_deleter_test.cpp b/src/mongo/db/s/collection_range_deleter_test.cpp
index 320b6b3796b..f6f6beb43c6 100644
--- a/src/mongo/db/s/collection_range_deleter_test.cpp
+++ b/src/mongo/db/s/collection_range_deleter_test.cpp
@@ -385,5 +385,34 @@ TEST_F(CollectionRangeDeleterTest, MultipleDocumentsInMultipleRangesToClean) {
     ASSERT_FALSE(next(rangeDeleter, 1));
 }
 
+// Tests that we retry on a WriteConflictException.
+TEST_F(CollectionRangeDeleterTest, RetryOnWriteConflictException) {
+    CollectionRangeDeleter rangeDeleter;
+    DBDirectClient dbclient(operationContext());
+
+    dbclient.insert(kNss.toString(), BSON(kShardKey << 1));
+    dbclient.insert(kNss.toString(), BSON(kShardKey << 2));
+    dbclient.insert(kNss.toString(), BSON(kShardKey << 3));
+    ASSERT_EQUALS(3ULL, dbclient.count(kNss.toString(), BSON(kShardKey << LT << 5)));
+
+    std::list<Deletion> ranges;
+    auto deletion = Deletion{ChunkRange(BSON(kShardKey << 0), BSON(kShardKey << 10)), Date_t{}};
+    ranges.emplace_back(std::move(deletion));
+    auto when = rangeDeleter.add(std::move(ranges));
+    ASSERT(when && *when == Date_t{});
+
+    // TODO SERVER-41606: Remove this function when we refactor CollectionRangeDeleter.
+    rangeDeleter.setDoDeletionShouldThrowWriteConflictForTest(true);
+
+    ASSERT_TRUE(next(rangeDeleter, 1));
+    ASSERT_EQUALS(3ULL, dbclient.count(kNss.toString(), BSON(kShardKey << LT << 5)));
+
+    // TODO SERVER-41606: Remove this function when we refactor CollectionRangeDeleter.
+    rangeDeleter.setDoDeletionShouldThrowWriteConflictForTest(false);
+
+    ASSERT_TRUE(next(rangeDeleter, 1));
+    ASSERT_EQUALS(2ULL, dbclient.count(kNss.toString(), BSON(kShardKey << LT << 5)));
+}
+
 }  // namespace
 }  // namespace mongo