From adae77e673bad94834e99d57e6ddeba7a2ac78c0 Mon Sep 17 00:00:00 2001
From: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Date: Tue, 10 Dec 2024 11:40:51 -0500
Subject: [PATCH] Addressing review

Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
---
 src/groups/mqb/mqbblp/mqbblp_cluster.cpp |  3 +++
 src/groups/mqb/mqbc/mqbc_storageutil.cpp | 13 +++++--------
 src/groups/mqb/mqbc/mqbc_storageutil.h   | 10 +++++-----
 src/groups/mqb/mqbi/mqbi_storage.h       |  5 +----
 src/groups/mqb/mqbs/mqbs_datastore.h     |  1 -
 5 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/src/groups/mqb/mqbblp/mqbblp_cluster.cpp b/src/groups/mqb/mqbblp/mqbblp_cluster.cpp
index 2723c0830..94a28ecef 100644
--- a/src/groups/mqb/mqbblp/mqbblp_cluster.cpp
+++ b/src/groups/mqb/mqbblp/mqbblp_cluster.cpp
@@ -2210,6 +2210,9 @@ void Cluster::onRecoveryStatusDispatched(
                 BSLS_ASSERT_SAFE(itMp->storage()->partitionId() ==
                                  static_cast<int>(pid));
 
+                // TODO:  wrong thread to call 'loadVirtualStorageDetails'
+                // but 'onRecoveryStatusDispatched' should not be concurrent
+                // with any of 'add/removeVirtualStorage' calls.
                 AppInfos appIdInfos;
                 itMp->storage()->loadVirtualStorageDetails(&appIdInfos);
 
diff --git a/src/groups/mqb/mqbc/mqbc_storageutil.cpp b/src/groups/mqb/mqbc/mqbc_storageutil.cpp
index 2c627eeec..7cca27ca0 100644
--- a/src/groups/mqb/mqbc/mqbc_storageutil.cpp
+++ b/src/groups/mqb/mqbc/mqbc_storageutil.cpp
@@ -155,12 +155,12 @@ void StorageUtil::loadAddedAndRemovedEntries(
     loadDifference(removedEntries, existingEntries, newEntries);
 }
 
-bool StorageUtil::loadUpdatedAppInfos(AppInfos* addedAppInfos,
-                                      AppInfos* removedAppInfos,
-                                      const mqbs::ReplicatedStorage& storage,
+bool StorageUtil::loadUpdatedAppInfos(AppInfos*       addedAppInfos,
+                                      AppInfos*       removedAppInfos,
+                                      const AppInfos& existingAppInfos,
                                       const AppInfos& newAppInfos)
 {
-    // executed by the *CLUSTER DISPATCHER* thread
+    // executed by the *QUEUE_DISPATCHER* thread
 
     // PRECONDITIONS
     BSLS_ASSERT_SAFE(addedAppInfos);
@@ -181,9 +181,6 @@ bool StorageUtil::loadUpdatedAppInfos(AppInfos* addedAppInfos,
     // list of newly added and removed appIds, and then invoking 'updateQueue'
     // in the appropriate thread.
 
-    AppInfos existingAppInfos;
-    storage.loadVirtualStorageDetails(&existingAppInfos);
-
     loadAddedAndRemovedEntries(addedAppInfos,
                                removedAppInfos,
                                existingAppInfos,
@@ -294,7 +291,7 @@ void StorageUtil::updateQueuePrimaryDispatched(
 
     bool hasUpdate = loadUpdatedAppInfos(&addedAppInfos,
                                          &removedAppInfos,
-                                         *storage,
+                                         existingAppInfos,
                                          appIdKeyPairs);
     if (!hasUpdate) {
         // No update needed for AppId/Key pairs.
diff --git a/src/groups/mqb/mqbc/mqbc_storageutil.h b/src/groups/mqb/mqbc/mqbc_storageutil.h
index 13abedc51..993bdf2d2 100644
--- a/src/groups/mqb/mqbc/mqbc_storageutil.h
+++ b/src/groups/mqb/mqbc/mqbc_storageutil.h
@@ -189,14 +189,14 @@ struct StorageUtil {
 
     /// Load into the specified `addedAppInfos` and
     /// `removedAppInfos` the appId/key pairs which have been added and
-    /// removed respectively for the specified `storage` based on the
+    /// removed respectively for the specified `existingAppInfos` based on the
     /// specified `newAppInfos`.  Return true if there are any added or removed
     /// appId/key pairs, false otherwise.
     ///
-    /// THREAD: Executed by the cluster dispatcher thread.
-    static bool loadUpdatedAppInfos(AppInfos* addedAppInfos,
-                                    AppInfos* removedAppInfos,
-                                    const mqbs::ReplicatedStorage& storage,
+    /// THREAD: Executed by the queue dispatcher thread.
+    static bool loadUpdatedAppInfos(AppInfos*       addedAppInfos,
+                                    AppInfos*       removedAppInfos,
+                                    const AppInfos& existingAppInfos,
                                     const AppInfos& newAppInfos);
 
     /// THREAD: Executed by the Queue's dispatcher thread.
diff --git a/src/groups/mqb/mqbi/mqbi_storage.h b/src/groups/mqb/mqbi/mqbi_storage.h
index 0574f2d15..95267c8af 100644
--- a/src/groups/mqb/mqbi/mqbi_storage.h
+++ b/src/groups/mqb/mqbi/mqbi_storage.h
@@ -375,10 +375,7 @@ class Storage {
   public:
     // PUBLIC TYPES
 
-    /// `AppInfo` is an alias for an (appId, appKey) pairing
-    /// representing unique virtual storage identification.
-
-    /// `AppInfos` is an alias for a set of pairs of appId and appKey
+    /// `AppInfos` is an alias for a map [appId] -> appKey
     typedef bsl::unordered_map<bsl::string, mqbu::StorageKey> AppInfos;
 
     typedef bmqc::Array<mqbu::StorageKey,
diff --git a/src/groups/mqb/mqbs/mqbs_datastore.h b/src/groups/mqb/mqbs/mqbs_datastore.h
index 99291a9f6..596a13f6f 100644
--- a/src/groups/mqb/mqbs/mqbs_datastore.h
+++ b/src/groups/mqb/mqbs/mqbs_datastore.h
@@ -272,7 +272,6 @@ class DataStoreConfigQueueInfo {
   public:
     // TYPES
     typedef mqbi::Storage::AppInfos AppInfos;
-    typedef AppInfos::const_iterator AppInfo;
 
   private:
     // DATA