SERVER-33766 Ensure secondaries always invalidate their in-memory routing table when new metadata arrives

5 files changed, 15 insertions, 36 deletions

diff --git a/src/mongo/db/s/collection_sharding_state.cpp b/src/mongo/db/s/collection_sharding_state.cpp
index eb2609beb75..1565e9d6fec 100644
--- a/src/mongo/db/s/collection_sharding_state.cpp
+++ b/src/mongo/db/s/collection_sharding_state.cpp
@@ -509,7 +509,8 @@ void CollectionShardingState::_onConfigCollectionsUpdateOp(OperationContext* opC
         // Need the WUOW to retain the lock for CollectionVersionLogOpHandler::commit().
         AutoGetCollection autoColl(opCtx, updatedNss, MODE_IX);
 
-        if (setField.hasField(ShardCollectionType::lastRefreshedCollectionVersion.name())) {
+        if (setField.hasField(ShardCollectionType::refreshing.name()) &&
+            !setField.getBoolField("refreshing")) {
             opCtx->recoveryUnit()->registerChange(
                 new CollectionVersionLogOpHandler(opCtx, updatedNss));
         }
diff --git a/src/mongo/db/s/shard_metadata_util.cpp b/src/mongo/db/s/shard_metadata_util.cpp
index 6d929554f30..fcf098fb6e0 100644
--- a/src/mongo/db/s/shard_metadata_util.cpp
+++ b/src/mongo/db/s/shard_metadata_util.cpp
@@ -87,13 +87,6 @@ std::string RefreshState::toString() const {
                          << lastRefreshedCollectionVersion.toString();
 }
 
-Status setPersistedRefreshFlags(OperationContext* opCtx, const NamespaceString& nss) {
-    // Set 'refreshing' to true.
-    BSONObj update = BSON(ShardCollectionType::refreshing() << true);
-    return updateShardCollectionsEntry(
-        opCtx, BSON(ShardCollectionType::ns() << nss.ns()), update, BSONObj(), false /*upsert*/);
-}
-
 Status unsetPersistedRefreshFlags(OperationContext* opCtx,
                                   const NamespaceString& nss,
                                   const ChunkVersion& refreshedVersion) {
@@ -185,8 +178,7 @@ Status updateShardCollectionsEntry(OperationContext* opCtx,
     invariant(query.hasField("_id"));
     if (upsert) {
         // If upserting, this should be an update from the config server that does not have shard
-        // refresh / migration inc signal information.
-        invariant(!update.hasField(ShardCollectionType::refreshing()));
+        // migration inc signal information.
         invariant(!update.hasField(ShardCollectionType::lastRefreshedCollectionVersion()));
         invariant(inc.isEmpty());
     }
diff --git a/src/mongo/db/s/shard_metadata_util.h b/src/mongo/db/s/shard_metadata_util.h
index 9bd035a41b6..0e04f029089 100644
--- a/src/mongo/db/s/shard_metadata_util.h
+++ b/src/mongo/db/s/shard_metadata_util.h
@@ -96,20 +96,6 @@ struct RefreshState {
 QueryAndSort createShardChunkDiffQuery(const ChunkVersion& collectionVersion);
 
 /**
- * Writes a persisted signal to indicate that the chunks collection is being updated. It is
- * essential to call this before updating the chunks collection for 'nss' so that secondaries do not
- * use incomplete metadata.
- *
- * It is safe to call this multiple times: it's an idempotent action.
- *
- * Don't forget to call refreshPersistedSignalFinish after the chunks update is finished!
- *
- * Note: if there is no document present in the collections collection for 'nss', nothing is
- * updated.
- */
-Status setPersistedRefreshFlags(OperationContext* opCtx, const NamespaceString& nss);
-
-/**
  * Writes a persisted signal to indicate that it is once again safe to read from the chunks
  * collection for 'nss' and updates the collection's collection version to 'refreshedVersion'. It is
  * essential to call this after updating the chunks collection so that secondaries know they can
@@ -146,8 +132,8 @@ StatusWith<ShardCollectionType> readShardCollectionsEntry(OperationContext* opCt
  * 'inc' can be used to specify fields and their increments: it will be assigned to the $inc
  * operator.
  *
- * If 'upsert' is true, expects neither 'refreshing' or 'lastRefreshedCollectionVersion' to be
- * present in the update: these refreshing fields should only be added to an existing document.
+ * If 'upsert' is true, expects lastRefreshedCollectionVersion' to be absent in the update:
+ * these refreshing fields should only be added to an existing document.
  * Similarly, 'inc' should not specify 'upsert' true.
  */
 Status updateShardCollectionsEntry(OperationContext* opCtx,
diff --git a/src/mongo/db/s/shard_metadata_util_test.cpp b/src/mongo/db/s/shard_metadata_util_test.cpp
index fed6991ed32..ae6e457ea3a 100644
--- a/src/mongo/db/s/shard_metadata_util_test.cpp
+++ b/src/mongo/db/s/shard_metadata_util_test.cpp
@@ -70,6 +70,8 @@ struct ShardMetadataUtilTest : public ShardServerTestFixture {
         ShardCollectionType shardCollectionType =
             assertGet(ShardCollectionType::fromBSON(builder.obj()));
 
+        shardCollectionType.setRefreshing(true);
+
         ASSERT_OK(updateShardCollectionsEntry(operationContext(),
                                               BSON(ShardCollectionType::ns(kNss.ns())),
                                               shardCollectionType.toBSON(),
@@ -194,9 +196,6 @@ TEST_F(ShardMetadataUtilTest, UpdateAndReadCollectionsEntry) {
 TEST_F(ShardMetadataUtilTest, PersistedRefreshSignalStartAndFinish) {
     setUpCollection();
 
-    // Signal refresh start
-    ASSERT_OK(setPersistedRefreshFlags(operationContext(), kNss));
-
     ShardCollectionType shardCollectionsEntry =
         assertGet(readShardCollectionsEntry(operationContext(), kNss));
 
@@ -210,7 +209,11 @@ TEST_F(ShardMetadataUtilTest, PersistedRefreshSignalStartAndFinish) {
     ASSERT(!shardCollectionsEntry.hasLastRefreshedCollectionVersion());
 
     // Signal refresh start again to make sure nothing changes
-    ASSERT_OK(setPersistedRefreshFlags(operationContext(), kNss));
+    ASSERT_OK(updateShardCollectionsEntry(operationContext(),
+                                          BSON(ShardCollectionType::ns() << kNss.ns()),
+                                          BSON(ShardCollectionType::refreshing() << true),
+                                          BSONObj(),
+                                          false));
 
     RefreshState state = assertGet(getPersistedRefreshFlags(operationContext(), kNss));
 
diff --git a/src/mongo/db/s/shard_server_catalog_cache_loader.cpp b/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
index fe07ce607d5..069685b1595 100644
--- a/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
+++ b/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
@@ -88,6 +88,9 @@ Status persistCollectionAndChangedChunks(OperationContext* opCtx,
                                                      collAndChunks.shardKeyPattern,
                                                      collAndChunks.defaultCollation,
                                                      collAndChunks.shardKeyIsUnique);
+    // Mark the chunk metadata as refreshing, so that secondaries are aware of refresh.
+    update.setRefreshing(true);
+
     Status status = updateShardCollectionsEntry(opCtx,
                                                 BSON(ShardCollectionType::ns() << nss.ns()),
                                                 update.toBSON(),
@@ -97,12 +100,6 @@ Status persistCollectionAndChangedChunks(OperationContext* opCtx,
         return status;
     }
 
-    // Mark the chunk metadata as refreshing, so that secondaries are aware of refresh.
-    status = setPersistedRefreshFlags(opCtx, nss);
-    if (!status.isOK()) {
-        return status;
-    }
-
     // Update the chunks.
     status = updateShardChunks(opCtx, nss, collAndChunks.changedChunks, collAndChunks.epoch);
     if (!status.isOK()) {