mqbc::StorageMgr, RecoveryMgr: Thread safety improvements #367

kaikulimu · 2024-07-22T20:11:53Z

This series of changes improved the thread safety of components integral to the FSM mode, including mqbc::StorageManager, mqbc::RecoveryManager, and other neighboring components.

hallfox · 2024-07-22T20:18:07Z

src/groups/mqb/mqbc/mqbc_clusterdata.h

@@ -79,13 +79,13 @@ class ClusterDataIdentity {
  private:
    // DATA

-    bsl::string d_name;
+    const bsl::string d_name;


I would avoid adding these consts here, this can lead to unusually restrictive class design issues later. If the intent is to ensure these fields are never modified after class construction, then we should provide an interface that guarantees that.

Good point. Our interface already has such guarantees, so I will remove the consts.

hallfox · 2024-07-22T20:18:39Z

src/groups/mqb/mqbc/mqbc_clusterdata.h

+    /// Get a modifiable reference to this object's event scheduler.
+    bdlmt::EventScheduler* scheduler();
+
+    /// Get a modifiable reference to this object's buffer factory.
+    bdlbb::BlobBufferFactory* bufferFactory();
+


Can the return value be NULL here? If not, should we even be returning a pointer type? We should specify.

No, they cannot be NULL here. I will add some asserts in the constructor, and change from pointer to reference here.

The issue is that mqbc::ClusterData itself only has pointers to components such as bdlmt::EventScheduler and bdlbb::BlobBufferFactory (see here). Take the example of bdlbb::BlobBufferFactory, the actual owner is mqba::Application::d_bufferFactory, and it's passed as pointer from mqba::Application -> mqbblp::ClusterCatalog -> mqbblp::Cluster -> mqbc::ClusterData. Given that, would it make sense to keep them as pointers?

Will also make copy constructor and copy assignment operator private, since this object is not supposed to be copyable.

678098 · 2024-07-23T10:53:59Z

src/groups/mqb/mqbc/mqbc_storagemanager.h

+    /// Initialize the queue key info map based on information in the specified
+    /// `clusterState`.
+    virtual void initializeQueueKeyInfoMap(
+        const mqbc::ClusterState* clusterState) BSLS_KEYWORD_OVERRIDE;


clusterState is a const parameter, it's not optional and required every time. Is it possible to convert it to const reference?

678098 · 2024-07-23T10:59:27Z

src/groups/mqb/mqbmock/mqbmock_storagemanager.cpp

+}
+
+void StorageManager::initializeQueueKeyInfoMap(
+    BSLS_ANNOTATION_UNUSED const mqbc::ClusterState* clusterState)


Alternatively you might remove the annotation and arg variable name to silence the unused variable compiler warning

Will keep it as BSLS_ANNOTATION_UNUSED to be consistent with other functions.

pniedzielski

I think I see most of the places where we've fixed thread-safety, but I have a few questions about some changes.

src/groups/mqb/mqbs/mqbs_filestore.cpp

src/groups/mqb/mqbc/mqbc_partitionstatetable.h

src/groups/mqb/mqbmock/mqbmock_storagemanager.cpp

src/groups/mqb/mqbblp/mqbblp_recoverymanager.cpp

src/groups/mqb/mqbc/mqbc_recoverymanager.cpp

pniedzielski · 2024-08-06T20:25:37Z

src/groups/mqb/mqbblp/mqbblp_storagemanager.cpp

+    // PRECONDITION
+    BSLS_ASSERT_SAFE(d_dispatcher_p->inDispatcherThread(d_cluster_p));
+
+    BSLS_ASSERT_OPT(false && "This method should only be invoked in FSM mode");


Is this really what we want? This pair of asserts seems wrong...the second one can fire even if we are in the dispatcher thread?

The only caller of initializeQueueKeyInfoMap() is mqbc::ClusterStateManager, and it should always use mqbc version of StorageManager. By design, only the FSM version of this method, mqbc::StorageManager::initializeQueueKeyInfoMap(), can be invoked. If you can think of a better way to express this, please let me know.

I see. In this case, the false makes sense. Perhaps, "Only the FSM version of this method from mqbc::StorageManager should be invoked." as the string?

Agreed. Will change

src/groups/mqb/mqbc/mqbc_storagemanager.cpp

src/groups/mqb/mqbi/mqbi_storagemanager.h

src/groups/mqb/mqbc/mqbc_storagemanager.t.cpp

src/groups/mqb/mqbblp/mqbblp_storagemanager.cpp

kaikulimu · 2024-08-23T21:30:35Z

@678098 @hallfox @pniedzielski Back for your review

pniedzielski

Changes look good to me!

bmq-oss-ci

Build 184 of commit 39fa4e7 has completed with FAILURE

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

bmq-oss-ci

Build 192 of commit d0301c8 has completed with FAILURE

) * mqbs:FileStore: Improve rc logging Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc::StorageManager.t: Remove the concept of replica healing stages Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbmock::StorageManager [new] Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc::ClusterData: Clean up manipulators and accessors Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc::RecoveryMgr: Thread safety improvements Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * Fix mqbc::StorageMgr: Initialize queue key info map in cluster thread Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc::StorageMgr.t: Set partition primary also in cluster state Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc::StorageMgr: Thread safety improvements Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * Apply clang-format Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * PR#367: Address feedback Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc::ClusterData: Return reference not pointer for non-nullables Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc: Make copy constructor and copy assignment private Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * blp::StorageManager: Improve assert logging Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbmock::StorageMgr: Fix compilation errors Signed-off-by: Yuan Jing Vincent Yan <[email protected]> * mqbc::RecoveryManager: Apply clang format Signed-off-by: Yuan Jing Vincent Yan <[email protected]> --------- Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

kaikulimu requested a review from a team as a code owner July 22, 2024 20:11

hallfox reviewed Jul 22, 2024

View reviewed changes

kaikulimu force-pushed the fsm-test__20240722__thread_safety branch from c754199 to f3d03d7 Compare July 22, 2024 20:35

678098 reviewed Jul 23, 2024

View reviewed changes

pniedzielski requested changes Aug 6, 2024

View reviewed changes

kaikulimu force-pushed the fsm-test__20240722__thread_safety branch from df6ca25 to 3bcd80b Compare August 23, 2024 20:39

kaikulimu assigned pniedzielski Aug 23, 2024

pniedzielski self-requested a review August 26, 2024 13:09

pniedzielski approved these changes Aug 26, 2024

View reviewed changes

bmq-oss-ci bot reviewed Aug 26, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

kaikulimu added 10 commits August 26, 2024 17:52

mqbs:FileStore: Improve rc logging

47e5133

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbc::StorageManager.t: Remove the concept of replica healing stages

db43def

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbmock::StorageManager [new]

23e8b25

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbc::ClusterData: Clean up manipulators and accessors

0300683

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbc::RecoveryMgr: Thread safety improvements

9a8b5b4

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

Fix mqbc::StorageMgr: Initialize queue key info map in cluster thread

b94c7e3

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbc::StorageMgr.t: Set partition primary also in cluster state

9ef2c9f

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbc::StorageMgr: Thread safety improvements

6dbad84

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

Apply clang-format

075a334

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

PR#367: Address feedback

d0aa95a

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

kaikulimu added 5 commits August 26, 2024 17:52

mqbc::ClusterData: Return reference not pointer for non-nullables

309c0b8

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbc: Make copy constructor and copy assignment private

d720c91

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

blp::StorageManager: Improve assert logging

b1de8eb

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbmock::StorageMgr: Fix compilation errors

91c4b02

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

mqbc::RecoveryManager: Apply clang format

d0301c8

Signed-off-by: Yuan Jing Vincent Yan <[email protected]>

kaikulimu force-pushed the fsm-test__20240722__thread_safety branch from 39fa4e7 to d0301c8 Compare August 26, 2024 21:53

bmq-oss-ci bot reviewed Aug 27, 2024

View reviewed changes

Merge branch 'main' into fsm-test__20240722__thread_safety

b03b245

kaikulimu merged commit 0f3466d into bloomberg:main Aug 28, 2024
24 checks passed

kaikulimu deleted the fsm-test__20240722__thread_safety branch August 29, 2024 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mqbc::StorageMgr, RecoveryMgr: Thread safety improvements #367

mqbc::StorageMgr, RecoveryMgr: Thread safety improvements #367

kaikulimu commented Jul 22, 2024

hallfox Jul 22, 2024

kaikulimu Jul 22, 2024

kaikulimu Aug 20, 2024

hallfox Jul 22, 2024

kaikulimu Jul 22, 2024 •

edited

Loading

kaikulimu Aug 20, 2024

kaikulimu Aug 21, 2024

kaikulimu Aug 23, 2024

678098 Jul 23, 2024

kaikulimu Aug 20, 2024

kaikulimu Aug 20, 2024

678098 Jul 23, 2024

kaikulimu Aug 20, 2024

kaikulimu Aug 20, 2024

pniedzielski left a comment

pniedzielski Aug 6, 2024

kaikulimu Aug 20, 2024 •

edited

Loading

pniedzielski Aug 21, 2024

kaikulimu Aug 21, 2024

kaikulimu Aug 23, 2024

kaikulimu commented Aug 23, 2024

pniedzielski left a comment

bmq-oss-ci bot left a comment

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

bmq-oss-ci bot left a comment

mqbc::StorageMgr, RecoveryMgr: Thread safety improvements #367

mqbc::StorageMgr, RecoveryMgr: Thread safety improvements #367

Conversation

kaikulimu commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikulimu Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pniedzielski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikulimu Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikulimu commented Aug 23, 2024

pniedzielski left a comment

Choose a reason for hiding this comment

bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

This comment was marked as duplicate.

bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

kaikulimu Jul 22, 2024 •

edited

Loading

kaikulimu Aug 20, 2024 •

edited

Loading