SERVER-53547 Make use of return values in ShardRegistry API

9 files changed, 41 insertions, 56 deletions

diff --git a/src/mongo/s/client/shard_registry.cpp b/src/mongo/s/client/shard_registry.cpp
index 93c26dd38df..bff4d96b849 100644
--- a/src/mongo/s/client/shard_registry.cpp
+++ b/src/mongo/s/client/shard_registry.cpp
@@ -349,23 +349,19 @@ StatusWith<std::shared_ptr<Shard>> ShardRegistry::getShard(OperationContext* opC
     return {ErrorCodes::ShardNotFound, str::stream() << "Shard " << shardId << " not found"};
 }
 
-void ShardRegistry::getAllShardIds(OperationContext* opCtx, std::vector<ShardId>* all) {
-    std::set<ShardId> seen;
-    auto data = _getData(opCtx);
-    data->getAllShardIds(seen);
-    if (seen.empty()) {
+std::vector<ShardId> ShardRegistry::getAllShardIds(OperationContext* opCtx) {
+    auto shardIds = _getData(opCtx)->getAllShardIds();
+    if (shardIds.empty()) {
         reload(opCtx);
-        data = _getData(opCtx);
-        data->getAllShardIds(seen);
+        shardIds = _getData(opCtx)->getAllShardIds();
     }
-    all->assign(seen.begin(), seen.end());
+    // Many logic in the codebase rely on this vector to be sorted.
+    std::sort(shardIds.begin(), shardIds.end());
+    return shardIds;
 }
 
 int ShardRegistry::getNumShards(OperationContext* opCtx) {
-    std::set<ShardId> seen;
-    auto data = _getData(opCtx);
-    data->getAllShardIds(seen);
-    return seen.size();
+    return _getData(opCtx)->getAllShardIds().size();
 }
 
 std::pair<std::vector<ShardRegistry::LatestConnStrings::value_type>, ShardRegistry::Increment>
@@ -581,18 +577,15 @@ std::shared_ptr<Shard> ShardRegistry::getShardForHostNoReload(const HostAndPort&
     return data->findByHostAndPort(host);
 }
 
-void ShardRegistry::getAllShardIdsNoReload(std::vector<ShardId>* all) const {
-    std::set<ShardId> seen;
-    auto data = _getCachedData();
-    data->getAllShardIds(seen);
-    all->assign(seen.begin(), seen.end());
+std::vector<ShardId> ShardRegistry::getAllShardIdsNoReload() const {
+    auto shardIds = _getCachedData()->getAllShardIds();
+    // Many logic in the codebase rely on this vector to be sorted.
+    std::sort(shardIds.begin(), shardIds.end());
+    return shardIds;
 }
 
 int ShardRegistry::getNumShardsNoReload() const {
-    std::set<ShardId> seen;
-    auto data = _getCachedData();
-    data->getAllShardIds(seen);
-    return seen.size();
+    return _getCachedData()->getAllShardIds().size();
 }
 
 std::shared_ptr<Shard> ShardRegistry::_getShardForRSNameNoReload(const std::string& name) const {
@@ -775,21 +768,23 @@ std::shared_ptr<Shard> ShardRegistryData::findShard(const ShardId& shardId) cons
     return nullptr;
 }
 
-void ShardRegistryData::getAllShards(std::vector<std::shared_ptr<Shard>>& result) const {
+std::vector<std::shared_ptr<Shard>> ShardRegistryData::getAllShards() const {
+    std::vector<std::shared_ptr<Shard>> result;
     result.reserve(_shardIdLookup.size());
     for (auto&& shard : _shardIdLookup) {
         result.emplace_back(shard.second);
     }
+    return result;
 }
 
-void ShardRegistryData::getAllShardIds(std::set<ShardId>& seen) const {
-    for (auto i = _shardIdLookup.begin(); i != _shardIdLookup.end(); ++i) {
-        const auto& s = i->second;
-        if (s->getId().toString() == "config") {
-            continue;
-        }
-        seen.insert(s->getId());
-    }
+std::vector<ShardId> ShardRegistryData::getAllShardIds() const {
+    std::vector<ShardId> shardIds;
+    shardIds.reserve(_shardIdLookup.size());
+    std::transform(_shardIdLookup.begin(),
+                   _shardIdLookup.end(),
+                   std::back_inserter(shardIds),
+                   [](const auto& shard) { return shard.second->getId(); });
+    return shardIds;
 }
 
 void ShardRegistryData::_addShard(std::shared_ptr<Shard> shard) {
@@ -833,8 +828,7 @@ void ShardRegistryData::_addShard(std::shared_ptr<Shard> shard) {
 void ShardRegistryData::toBSON(BSONObjBuilder* map,
                                BSONObjBuilder* hosts,
                                BSONObjBuilder* connStrings) const {
-    std::vector<std::shared_ptr<Shard>> shards;
-    getAllShards(shards);
+    auto shards = getAllShards();
 
     std::sort(std::begin(shards),
               std::end(shards),
@@ -862,8 +856,7 @@ void ShardRegistryData::toBSON(BSONObjBuilder* map,
 }
 
 void ShardRegistryData::toBSON(BSONObjBuilder* result) const {
-    std::vector<std::shared_ptr<Shard>> shards;
-    getAllShards(shards);
+    auto shards = getAllShards();
 
     std::sort(std::begin(shards),
               std::end(shards),
diff --git a/src/mongo/s/client/shard_registry.h b/src/mongo/s/client/shard_registry.h
index 4a4d791cfe1..78f433e728b 100644
--- a/src/mongo/s/client/shard_registry.h
+++ b/src/mongo/s/client/shard_registry.h
@@ -111,14 +111,14 @@ public:
     std::shared_ptr<Shard> findByHostAndPort(const HostAndPort&) const;
 
     /**
-     * Returns the set of all known shard ids.
+     * Returns an unsorted vector of all known shard ids.
      */
-    void getAllShardIds(std::set<ShardId>& result) const;
+    std::vector<ShardId> getAllShardIds() const;
 
     /**
-     * Returns the set of all known shard objects.
+     * Returns an unsorted vector of all known shard objects.
      */
-    void getAllShards(std::vector<std::shared_ptr<Shard>>& result) const;
+    std::vector<std::shared_ptr<Shard>> getAllShards() const;
 
     void toBSON(BSONObjBuilder* result) const;
     void toBSON(BSONObjBuilder* map, BSONObjBuilder* hosts, BSONObjBuilder* connStrings) const;
@@ -238,9 +238,9 @@ public:
     StatusWith<std::shared_ptr<Shard>> getShard(OperationContext* opCtx, const ShardId& shardId);
 
     /**
-     * Populates all known shard ids into the given vector.
+     * Return a sorted vector containing all known shard ids.
      */
-    void getAllShardIds(OperationContext* opCtx, std::vector<ShardId>* all);
+    std::vector<ShardId> getAllShardIds(OperationContext* opCtx);
 
     /**
      * Returns the number of shards.
@@ -314,7 +314,7 @@ public:
      */
     std::shared_ptr<Shard> getShardForHostNoReload(const HostAndPort& shardHost) const;
 
-    void getAllShardIdsNoReload(std::vector<ShardId>* all) const;
+    std::vector<ShardId> getAllShardIdsNoReload() const;
 
     int getNumShardsNoReload() const;
 
diff --git a/src/mongo/s/cluster_commands_helpers.cpp b/src/mongo/s/cluster_commands_helpers.cpp
index d2fa87d5979..21c65484811 100644
--- a/src/mongo/s/cluster_commands_helpers.cpp
+++ b/src/mongo/s/cluster_commands_helpers.cpp
@@ -164,8 +164,7 @@ std::vector<AsyncRequestsSender::Request> buildUnshardedRequestsForAllShards(
 
 std::vector<AsyncRequestsSender::Request> buildUnversionedRequestsForAllShards(
     OperationContext* opCtx, const BSONObj& cmdObj) {
-    std::vector<ShardId> shardIds;
-    Grid::get(opCtx)->shardRegistry()->getAllShardIdsNoReload(&shardIds);
+    auto shardIds = Grid::get(opCtx)->shardRegistry()->getAllShardIdsNoReload();
     return buildUnshardedRequestsForAllShards(opCtx, std::move(shardIds), cmdObj);
 }
 
diff --git a/src/mongo/s/commands/cluster_fsync_cmd.cpp b/src/mongo/s/commands/cluster_fsync_cmd.cpp
index c25580d95de..e82469acee3 100644
--- a/src/mongo/s/commands/cluster_fsync_cmd.cpp
+++ b/src/mongo/s/commands/cluster_fsync_cmd.cpp
@@ -82,8 +82,7 @@ public:
         bool ok = true;
 
         auto const shardRegistry = Grid::get(opCtx)->shardRegistry();
-        std::vector<ShardId> shardIds;
-        shardRegistry->getAllShardIdsNoReload(&shardIds);
+        const auto shardIds = shardRegistry->getAllShardIdsNoReload();
 
         for (const ShardId& shardId : shardIds) {
             auto shardStatus = shardRegistry->getShard(opCtx, shardId);
diff --git a/src/mongo/s/commands/cluster_list_databases_cmd.cpp b/src/mongo/s/commands/cluster_list_databases_cmd.cpp
index e6aaa7aec5c..d4a9cec8b9e 100644
--- a/src/mongo/s/commands/cluster_list_databases_cmd.cpp
+++ b/src/mongo/s/commands/cluster_list_databases_cmd.cpp
@@ -122,8 +122,7 @@ public:
         std::map<std::string, long long> sizes;
         std::map<std::string, std::unique_ptr<BSONObjBuilder>> dbShardInfo;
 
-        std::vector<ShardId> shardIds;
-        shardRegistry->getAllShardIdsNoReload(&shardIds);
+        auto shardIds = shardRegistry->getAllShardIdsNoReload();
         shardIds.emplace_back(ShardId::kConfigServerId);
 
         // { filter: matchExpression }.
diff --git a/src/mongo/s/commands/cluster_multicast_cmd.cpp b/src/mongo/s/commands/cluster_multicast_cmd.cpp
index d4d283e4b13..834a786769f 100644
--- a/src/mongo/s/commands/cluster_multicast_cmd.cpp
+++ b/src/mongo/s/commands/cluster_multicast_cmd.cpp
@@ -47,8 +47,7 @@ namespace {
 std::vector<HostAndPort> getAllClusterHosts(OperationContext* opCtx) {
     auto registry = Grid::get(opCtx)->shardRegistry();
 
-    std::vector<ShardId> shardIds;
-    registry->getAllShardIds(opCtx, &shardIds);
+    const auto shardIds = registry->getAllShardIds(opCtx);
 
     std::vector<HostAndPort> servers;
     for (const auto& shardId : shardIds) {
diff --git a/src/mongo/s/commands/cluster_shard_collection_cmd.cpp b/src/mongo/s/commands/cluster_shard_collection_cmd.cpp
index 1e61995dbad..2c26ba02add 100644
--- a/src/mongo/s/commands/cluster_shard_collection_cmd.cpp
+++ b/src/mongo/s/commands/cluster_shard_collection_cmd.cpp
@@ -71,9 +71,7 @@ void createCollection(OperationContext* opCtx,
 
     ShardId shardId;
     if (nss.db() == NamespaceString::kConfigDb) {
-        std::vector<ShardId> shardIds;
-        auto shardRegistry = Grid::get(opCtx)->shardRegistry();
-        shardRegistry->getAllShardIds(opCtx, &shardIds);
+        const auto shardIds = Grid::get(opCtx)->shardRegistry()->getAllShardIds(opCtx);
         uassert(ErrorCodes::IllegalOperation, "there are no shards to target", !shardIds.empty());
         shardId = shardIds[0];
     } else {
diff --git a/src/mongo/s/commands/kill_sessions_remote.cpp b/src/mongo/s/commands/kill_sessions_remote.cpp
index 0bda8eacf61..3e588f0fa9d 100644
--- a/src/mongo/s/commands/kill_sessions_remote.cpp
+++ b/src/mongo/s/commands/kill_sessions_remote.cpp
@@ -53,8 +53,7 @@ namespace {
 std::vector<HostAndPort> getAllClusterHosts(OperationContext* opCtx) {
     auto registry = Grid::get(opCtx)->shardRegistry();
 
-    std::vector<ShardId> shardIds;
-    registry->getAllShardIds(opCtx, &shardIds);
+    const auto shardIds = registry->getAllShardIds(opCtx);
 
     std::vector<HostAndPort> servers;
     for (const auto& shardId : shardIds) {
diff --git a/src/mongo/s/write_ops/chunk_manager_targeter.cpp b/src/mongo/s/write_ops/chunk_manager_targeter.cpp
index 87a2ab64c6e..7c3b2fb8a27 100644
--- a/src/mongo/s/write_ops/chunk_manager_targeter.cpp
+++ b/src/mongo/s/write_ops/chunk_manager_targeter.cpp
@@ -576,8 +576,7 @@ std::vector<ShardEndpoint> ChunkManagerTargeter::targetAllShards(OperationContex
     // implies the collection is sharded, so we should always have a chunk manager.
     invariant(_cm->isSharded());
 
-    std::vector<ShardId> shardIds;
-    Grid::get(opCtx)->shardRegistry()->getAllShardIdsNoReload(&shardIds);
+    auto shardIds = Grid::get(opCtx)->shardRegistry()->getAllShardIdsNoReload();
 
     std::vector<ShardEndpoint> endpoints;
     for (auto&& shardId : shardIds) {