Refactoring App registration #493

dorjesinpo · 2024-10-30T22:40:32Z

Fix the race when reconfiguring domain in FSM
Generate AppKey only once when reconfiguring domain.

bmq-oss-ci

Build 356 of commit d687443 has completed with FAILURE

src/groups/mqb/mqbblp/mqbblp_rootqueueengine.h

kaikulimu · 2024-11-06T20:43:24Z

src/groups/mqb/mqbblp/mqbblp_rootqueueengine.cpp

-                       << "never be called for a non-Fanout queue.  Received "
-                       << "call to register appId '" << appIdKeyPair.first
-                       << "', appKey '" << appIdKeyPair.second << "'.";
+                       << "never be called for a non-Fanout queue.";


Why did you remove printing of appId and appKey? They help with debugging.

In fact, it could help to also print the queue uri here.

This is no longer singular. Not sure we want to print the entire collection. Note that this code never executes.

Just printing the queue uri would be nice.

kaikulimu · 2024-11-06T23:56:56Z

src/groups/mqb/mqbblp/mqbblp_rootqueueengine.cpp

-        BALL_LOG_ERROR << "It should be not possible to unregister appId '"
-                       << appIdKeyPair.first << "', appKey '"
-                       << appIdKeyPair.second << "' for a non-Fanout queue.";
+        BALL_LOG_ERROR << "Invalid queue type for unregistering appId.";


Printing queue uri, offending appId and appKey can be helpful

Same comment

Just printing the queue uri would be nice.

src/groups/mqb/mqbblp/mqbblp_rootqueueengine.h

kaikulimu · 2024-11-07T00:11:52Z

src/groups/mqb/mqbi/mqbi_queueengine.cpp

 {
    // NOTHING
 }

 void QueueEngine::afterAppIdUnregistered(
-    BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfo& appIdKeyPair)
+    BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfos& appIdKeyPairs)


Suggested change

BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfos& appIdKeyPairs)

BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfos& removedAppIds)

to be consistent with RootQE

Still needs to be changed in this .cpp file.

src/integration-tests/test_appids.py

kaikulimu · 2024-11-08T20:01:13Z

Added thought, I think we should never generate appKey at

blazingmq/src/groups/mqb/mqbc/mqbc_storageutil.cpp

Lines 2444 to 2475 in 613c5ba

    
           if (cluster->isCSLModeEnabled() || !appIdKeyPairs.empty()) { 
        
               for (AppInfosCIter citer = appIdKeyPairs.begin(); 
        
                    citer != appIdKeyPairs.end(); 
        
                    ++citer) { 
        
                   rc = storageSp->addVirtualStorage(errorDesc, 
        
                                                     citer->first, 
        
                                                     citer->second); 
        
               } 
        
               appIdKeyPairsToUse = appIdKeyPairs; 
        
           } 
        
           else { 
        
               // If fanout queue, generate unique appKeys for the configured 
        
               // appIds, and create virtual storages for them.  Caller expects 
        
               // virtual storages to be created when 'registerQueue' returns. 
        
               const mqbconfm::QueueModeFanout& fanoutMode = queueMode.fanout(); 
        
               const bsl::vector<bsl::string>&  cfgAppIds  = fanoutMode.appIDs(); 
        
               typedef bsl::vector<bsl::string>::const_iterator ConstIter; 
        
               for (ConstIter citer = cfgAppIds.begin(); citer != cfgAppIds.end(); 
        
                    ++citer) { 
        
                   mqbu::StorageKey appKey = generateAppKey(appKeys, 
        
                                                            appKeysLock, 
        
                                                            *citer); 
        
                   rc = storageSp->addVirtualStorage(errorDesc, *citer, appKey); 
        
                   appIdKeyPairsToUse.emplace(bsl::make_pair(*citer, appKey)); 
        
               } 
        
           }

Our end goal is to only generate appKey at the CSL layer.

Be courageous and not be afraid of changing existing code :)

Signed-off-by: dorjesinpo <[email protected]>

dorjesinpo · 2024-11-08T21:15:44Z

Added thought, I think we should never generate appKey at
We have been over it at the previous commit when we have added || !appIdKeyPairs.empty()

bmq-oss-ci

Build 366 of commit cdd9c32 has completed with FAILURE

kaikulimu

Minor comments

kaikulimu · 2024-11-11T17:03:33Z

src/groups/mqb/mqbblp/mqbblp_rootqueueengine.cpp

-                       << "never be called for a non-Fanout queue.  Received "
-                       << "call to register appId '" << appIdKeyPair.first
-                       << "', appKey '" << appIdKeyPair.second << "'.";
+                       << "never be called for a non-Fanout queue.";


Just printing the queue uri would be nice.

kaikulimu · 2024-11-11T17:04:01Z

src/groups/mqb/mqbblp/mqbblp_rootqueueengine.cpp

-        BALL_LOG_ERROR << "It should be not possible to unregister appId '"
-                       << appIdKeyPair.first << "', appKey '"
-                       << appIdKeyPair.second << "' for a non-Fanout queue.";
+        BALL_LOG_ERROR << "Invalid queue type for unregistering appId.";


Just printing the queue uri would be nice.

kaikulimu · 2024-11-11T17:06:49Z

src/groups/mqb/mqbi/mqbi_queueengine.cpp

 {
    // NOTHING
 }

 void QueueEngine::afterAppIdUnregistered(
-    BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfo& appIdKeyPair)
+    BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfos& appIdKeyPairs)


Still needs to be changed in this .cpp file.

kaikulimu · 2024-11-11T17:07:16Z

src/groups/mqb/mqbi/mqbi_queueengine.cpp

@@ -36,13 +36,13 @@ QueueEngine::~QueueEngine()
 }

 void QueueEngine::afterAppIdRegistered(
-    BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfo& appIdKeyPair)
+    BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfos& appIdKeyPairs)


Still needs to be changed in this .cpp file.

Signed-off-by: dorjesinpo <[email protected]>

kaikulimu

lgtm

dorjesinpo requested a review from a team as a code owner October 30, 2024 22:40

dorjesinpo force-pushed the dev/squeaky-2 branch 5 times, most recently from 7b9493f to b0f355e Compare November 4, 2024 19:49

dorjesinpo changed the title ~~WIP~~ Refactoring App registration Nov 4, 2024

dorjesinpo force-pushed the dev/squeaky-2 branch from b0f355e to d687443 Compare November 4, 2024 19:55

dorjesinpo requested a review from kaikulimu November 4, 2024 21:28

dorjesinpo assigned kaikulimu Nov 4, 2024

bmq-oss-ci bot reviewed Nov 4, 2024

View reviewed changes

kaikulimu requested changes Nov 8, 2024

View reviewed changes

kaikulimu assigned dorjesinpo and unassigned kaikulimu Nov 8, 2024

Refactoring App registration

181ecbb

Signed-off-by: dorjesinpo <[email protected]>

dorjesinpo force-pushed the dev/squeaky-2 branch from d95d809 to 9a5a99a Compare November 8, 2024 21:09

dorjesinpo force-pushed the dev/squeaky-2 branch from 9a5a99a to cdd9c32 Compare November 8, 2024 21:16

dorjesinpo assigned kaikulimu and unassigned dorjesinpo Nov 8, 2024

bmq-oss-ci bot reviewed Nov 8, 2024

View reviewed changes

kaikulimu assigned dorjesinpo and unassigned kaikulimu Nov 11, 2024

kaikulimu reviewed Nov 11, 2024

View reviewed changes

Cleaning

59c7e54

Signed-off-by: dorjesinpo <[email protected]>

dorjesinpo force-pushed the dev/squeaky-2 branch from cdd9c32 to 59c7e54 Compare November 11, 2024 17:28

dorjesinpo assigned kaikulimu and unassigned dorjesinpo Nov 11, 2024

kaikulimu approved these changes Nov 11, 2024

View reviewed changes

dorjesinpo merged commit 759da3c into main Nov 11, 2024
30 of 32 checks passed

kaikulimu mentioned this pull request Nov 11, 2024

Fix[MQB]: Enable strong consistency CSL and fix leader activeness #495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring App registration #493

Refactoring App registration #493

dorjesinpo commented Oct 30, 2024

bmq-oss-ci bot left a comment

kaikulimu Nov 6, 2024

dorjesinpo Nov 8, 2024

kaikulimu Nov 11, 2024

kaikulimu Nov 6, 2024

dorjesinpo Nov 8, 2024

kaikulimu Nov 11, 2024

kaikulimu Nov 7, 2024

kaikulimu Nov 11, 2024

kaikulimu commented Nov 8, 2024 •

edited

Loading

dorjesinpo commented Nov 8, 2024

bmq-oss-ci bot left a comment

kaikulimu left a comment

kaikulimu Nov 11, 2024

kaikulimu Nov 11, 2024

kaikulimu Nov 11, 2024

kaikulimu Nov 11, 2024

kaikulimu left a comment

	BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfos& appIdKeyPairs)
	BSLS_ANNOTATION_UNUSED const mqbi::Storage::AppInfos& removedAppIds)

Refactoring App registration #493

Refactoring App registration #493

Conversation

dorjesinpo commented Oct 30, 2024

bmq-oss-ci bot left a 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

kaikulimu commented Nov 8, 2024 • edited Loading

dorjesinpo commented Nov 8, 2024

bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

kaikulimu left a 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

kaikulimu left a comment

Choose a reason for hiding this comment

kaikulimu commented Nov 8, 2024 •

edited

Loading