SERVER-37918 Improve the messaging around corrupted routing metadata

5 files changed, 64 insertions, 59 deletions

diff --git a/src/mongo/s/catalog/type_chunk.cpp b/src/mongo/s/catalog/type_chunk.cpp
index 5504666beb0..4965975b8a0 100644
--- a/src/mongo/s/catalog/type_chunk.cpp
+++ b/src/mongo/s/catalog/type_chunk.cpp
@@ -472,6 +472,14 @@ Status ChunkType::validate() const {
                 str::stream() << "max is not greater than min: " << *_min << ", " << *_max};
     }
 
+    if (!_history.empty()) {
+        if (_history.front().getShard() != *_shard) {
+            return {ErrorCodes::BadValue,
+                    str::stream() << "History contains an invalid shard "
+                                  << _history.front().getShard()};
+        }
+    }
+
     return Status::OK();
 }
 
diff --git a/src/mongo/s/chunk.cpp b/src/mongo/s/chunk.cpp
index 14d01790013..f5bdf08804b 100644
--- a/src/mongo/s/chunk.cpp
+++ b/src/mongo/s/chunk.cpp
@@ -47,10 +47,7 @@ ChunkInfo::ChunkInfo(const ChunkType& from)
       _history(from.getHistory()),
       _jumbo(from.getJumbo()),
       _writesTracker(std::make_shared<ChunkWritesTracker>()) {
-    invariant(from.validate());
-    if (!_history.empty()) {
-        invariant(_shardId == _history.front().getShard());
-    }
+    uassertStatusOK(from.validate());
 }
 
 const ShardId& ChunkInfo::getShardIdAt(const boost::optional<Timestamp>& ts) const {
diff --git a/src/mongo/s/chunk.h b/src/mongo/s/chunk.h
index 896d658745d..1f267d3525f 100644
--- a/src/mongo/s/chunk.h
+++ b/src/mongo/s/chunk.h
@@ -46,6 +46,10 @@ class ChunkInfo {
 public:
     explicit ChunkInfo(const ChunkType& from);
 
+    const auto& getRange() const {
+        return _range;
+    }
+
     const BSONObj& getMin() const {
         return _range.getMin();
     }
diff --git a/src/mongo/s/chunk_manager.cpp b/src/mongo/s/chunk_manager.cpp
index b9e150ce85f..ca5e9a61cad 100644
--- a/src/mongo/s/chunk_manager.cpp
+++ b/src/mongo/s/chunk_manager.cpp
@@ -86,9 +86,8 @@ RoutingTableHistory::RoutingTableHistory(NamespaceString nss,
       _defaultCollator(std::move(defaultCollator)),
       _unique(unique),
       _chunkMap(std::move(chunkMap)),
-      _shardVersions(
-          _constructShardVersionMap(collectionVersion.epoch(), _chunkMap, _shardKeyOrdering)),
-      _collectionVersion(collectionVersion) {}
+      _collectionVersion(collectionVersion),
+      _shardVersions(_constructShardVersionMap()) {}
 
 Chunk ChunkManager::findIntersectingChunk(const BSONObj& shardKey, const BSONObj& collation) const {
     const bool hasSimpleCollation = (collation.isEmpty() && !_rt->getDefaultCollator()) ||
@@ -406,66 +405,65 @@ std::string RoutingTableHistory::toString() const {
     return sb.str();
 }
 
-ShardVersionMap RoutingTableHistory::_constructShardVersionMap(const OID& epoch,
-                                                               const ChunkInfoMap& chunkMap,
-                                                               Ordering shardKeyOrdering) {
+ShardVersionMap RoutingTableHistory::_constructShardVersionMap() const {
+    const OID& epoch = _collectionVersion.epoch();
+
     ShardVersionMap shardVersions;
-    ChunkInfoMap::const_iterator current = chunkMap.cbegin();
+    ChunkInfoMap::const_iterator current = _chunkMap.cbegin();
 
     boost::optional<BSONObj> firstMin = boost::none;
     boost::optional<BSONObj> lastMax = boost::none;
 
-    while (current != chunkMap.cend()) {
+    while (current != _chunkMap.cend()) {
         const auto& firstChunkInRange = current->second;
+        const auto& currentRangeShardId = firstChunkInRange->getShardIdAt(boost::none);
 
         // Tracks the max shard version for the shard on which the current range will reside
-        auto shardVersionIt = shardVersions.find(firstChunkInRange->getShardIdAt(boost::none));
+        auto shardVersionIt = shardVersions.find(currentRangeShardId);
         if (shardVersionIt == shardVersions.end()) {
-            shardVersionIt = shardVersions
-                                 .emplace(firstChunkInRange->getShardIdAt(boost::none),
-                                          ChunkVersion(0, 0, epoch))
-                                 .first;
+            shardVersionIt =
+                shardVersions.emplace(currentRangeShardId, ChunkVersion(0, 0, epoch)).first;
         }
 
         auto& maxShardVersion = shardVersionIt->second;
 
-        current = std::find_if(
-            current,
-            chunkMap.cend(),
-            [&firstChunkInRange, &maxShardVersion](const ChunkInfoMap::value_type& chunkMapEntry) {
-                const auto& currentChunk = chunkMapEntry.second;
+        current =
+            std::find_if(current,
+                         _chunkMap.cend(),
+                         [&currentRangeShardId,
+                          &maxShardVersion](const ChunkInfoMap::value_type& chunkMapEntry) {
+                             const auto& currentChunk = chunkMapEntry.second;
 
-                if (currentChunk->getShardIdAt(boost::none) !=
-                    firstChunkInRange->getShardIdAt(boost::none))
-                    return true;
+                             if (currentChunk->getShardIdAt(boost::none) != currentRangeShardId)
+                                 return true;
 
-                if (currentChunk->getLastmod() > maxShardVersion)
-                    maxShardVersion = currentChunk->getLastmod();
+                             if (currentChunk->getLastmod() > maxShardVersion)
+                                 maxShardVersion = currentChunk->getLastmod();
 
-                return false;
-            });
+                             return false;
+                         });
 
         const auto rangeLast = std::prev(current);
 
-        const BSONObj rangeMin = firstChunkInRange->getMin();
-        const BSONObj rangeMax = rangeLast->second->getMax();
-
-        if (lastMax) {
-            uassert(ErrorCodes::ConflictingOperationInProgress,
-                    str::stream()
-                        << "Metadata contains chunks with the same or out-of-order max value; "
-                           "expected "
-                        << lastMax.get()
-                        << " < "
-                        << rangeMax,
-                    SimpleBSONObjComparator::kInstance.evaluate(lastMax.get() < rangeMax));
-            // Make sure there are no gaps in the ranges
-            uassert(ErrorCodes::ConflictingOperationInProgress,
-                    str::stream() << "Gap or an overlap between ranges "
-                                  << ChunkRange(rangeMin, rangeMax).toString()
-                                  << " and "
-                                  << lastMax.get(),
-                    SimpleBSONObjComparator::kInstance.evaluate(lastMax.get() == rangeMin));
+        const auto& rangeMin = firstChunkInRange->getMin();
+        const auto& rangeMax = rangeLast->second->getMax();
+
+        // Check the continuity of the chunks map
+        if (lastMax && !SimpleBSONObjComparator::kInstance.evaluate(*lastMax == rangeMin)) {
+            if (SimpleBSONObjComparator::kInstance.evaluate(*lastMax < rangeMin))
+                uasserted(ErrorCodes::ConflictingOperationInProgress,
+                          str::stream()
+                              << "Gap exists in the routing table between chunks "
+                              << _chunkMap.at(_extractKeyString(*lastMax))->getRange().toString()
+                              << " and "
+                              << rangeLast->second->getRange().toString());
+            else
+                uasserted(ErrorCodes::ConflictingOperationInProgress,
+                          str::stream()
+                              << "Overlap exists in the routing table between chunks "
+                              << _chunkMap.at(_extractKeyString(*lastMax))->getRange().toString()
+                              << " and "
+                              << rangeLast->second->getRange().toString());
         }
 
         if (!firstMin)
@@ -478,7 +476,7 @@ ShardVersionMap RoutingTableHistory::_constructShardVersionMap(const OID& epoch,
         invariant(maxShardVersion.isSet());
     }
 
-    if (!chunkMap.empty()) {
+    if (!_chunkMap.empty()) {
         invariant(!shardVersions.empty());
         invariant(firstMin.is_initialized());
         invariant(lastMax.is_initialized());
diff --git a/src/mongo/s/chunk_manager.h b/src/mongo/s/chunk_manager.h
index d8cbabb60d5..30ef581dddc 100644
--- a/src/mongo/s/chunk_manager.h
+++ b/src/mongo/s/chunk_manager.h
@@ -152,13 +152,6 @@ public:
 
 
 private:
-    /**
-     * Does a single pass over the chunkMap and constructs the ShardVersionMap object.
-     */
-    static ShardVersionMap _constructShardVersionMap(const OID& epoch,
-                                                     const ChunkInfoMap& chunkMap,
-                                                     Ordering shardKeyOrdering);
-
     RoutingTableHistory(NamespaceString nss,
                         boost::optional<UUID> uuid,
                         KeyPattern shardKeyPattern,
@@ -167,6 +160,11 @@ private:
                         ChunkInfoMap chunkMap,
                         ChunkVersion collectionVersion);
 
+    /**
+     * Does a single pass over the chunkMap and constructs the ShardVersionMap object.
+     */
+    ShardVersionMap _constructShardVersionMap() const;
+
     std::string _extractKeyString(const BSONObj& shardKeyValue) const;
 
     // The shard versioning mechanism hinges on keeping track of the number of times we reload
@@ -194,13 +192,13 @@ private:
     // ranges must cover the complete space from [MinKey, MaxKey).
     const ChunkInfoMap _chunkMap;
 
+    // Max version across all chunks
+    const ChunkVersion _collectionVersion;
+
     // Map from shard id to the maximum chunk version for that shard. If a shard contains no
     // chunks, it won't be present in this map.
     const ShardVersionMap _shardVersions;
 
-    // Max version across all chunks
-    const ChunkVersion _collectionVersion;
-
     friend class ChunkManager;
 };