SERVER-33766 Ensure that secondaries always invalidate the in-memory routing table when new

metadata arrives

5 files changed, 16 insertions, 35 deletions

diff --git a/src/mongo/db/s/shard_metadata_util.cpp b/src/mongo/db/s/shard_metadata_util.cpp
index 21afbf7b306..8f4e3dcf3c1 100644
--- a/src/mongo/db/s/shard_metadata_util.cpp
+++ b/src/mongo/db/s/shard_metadata_util.cpp
@@ -88,13 +88,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) {
@@ -218,7 +211,6 @@ Status updateShardCollectionsEntry(OperationContext* opCtx,
     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()));
         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 62c6c68262e..bce20a05e38 100644
--- a/src/mongo/db/s/shard_metadata_util.h
+++ b/src/mongo/db/s/shard_metadata_util.h
@@ -97,20 +97,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
@@ -152,8 +138,8 @@ StatusWith<ShardDatabaseType> readShardDatabasesEntry(OperationContext* opCtx, S
  * '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 0fc7d24d5b6..f6596c1230a 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 7943d6b8265..8008640e57c 100644
--- a/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
+++ b/src/mongo/db/s/shard_server_catalog_cache_loader.cpp
@@ -89,6 +89,10 @@ 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(),
@@ -98,12 +102,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()) {
diff --git a/src/mongo/db/s/shard_server_op_observer.cpp b/src/mongo/db/s/shard_server_op_observer.cpp
index a9fcd81a883..2518f899fac 100644
--- a/src/mongo/db/s/shard_server_op_observer.cpp
+++ b/src/mongo/db/s/shard_server_op_observer.cpp
@@ -231,6 +231,7 @@ void ShardServerOpObserver::onInserts(OperationContext* opCtx,
 void ShardServerOpObserver::onUpdate(OperationContext* opCtx, const OplogUpdateEntryArgs& args) {
     auto const css = CollectionShardingState::get(opCtx, args.nss);
     const auto metadata = css->getMetadata(opCtx);
+    log() << "XXX on udpate op observer";
 
     if (args.nss.ns() == NamespaceString::kShardConfigCollectionsCollectionName) {
         // Notification of routing table changes are only needed on secondaries
@@ -270,7 +271,8 @@ void ShardServerOpObserver::onUpdate(OperationContext* opCtx, const OplogUpdateE
             // 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));
             }