Feat: CQH calls Queue::getHandle #565

dorjesinpo · 2025-01-13T15:19:05Z

For the upcoming Dynamic Apps feature, we need ClusterQueueHelper to be the caller of Queue::getHandle (and inspect the status to make Dynamic Apps updates if necessary).
So, instead of Domain calling Queue::getHandle after Cluster::openQueue, it is the Cluster (ClusterQueueHelper) calling Queue::getHandle as part of Cluster::openQueue.

Roughly, the sequence looks like this:

    //  CQH::processPeerOpenQueueRequest  QueueSessionManager::processOpenQueue
    //              \                       /
    //               V                     V
    //              DomainFactory::createDomain
    //              /                      \
    //             V                        V
    //  CQH::onGetDomain                  QueueSessionManager::onDomainOpenCb
    //          |                           /
    //          V                          /
    //      CQH::onGetDomainDispatched    /
    //                      \            /
    //                       V          V
    //                      Domain::openQueue
    //                              |
    //                              V
    //                          Cluster::openQueue
    //                              |
    //                              V
    //                              CQH::openQueue
    //                                      \
    //                                       V
    //                                  Queue::getHandle
    //                                      /
    //                                     V
    //                              CQH::onGetQueueHandle. (future Dynamic Apps work)
    //                                    /
    //                                   V
    //                      Domain::onOpenQueueResponse
    //                        /                 \
    //                       V                   V
    //  CQH::onGetQueueHandleDispatched     QueueSessionManager::onQueueOpenCb
    //
    ```

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

678098 · 2025-01-13T17:34:46Z

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

+    const bsl::shared_ptr<mqbi::QueueHandleRequesterContext>& clientContext,
+    const mqbi::Cluster::OpenQueueCallback&                   callback)
+: d_queueContext_p(0)
+, d_domain_p(domain)


The domain might be NULL if we are in the proxy, right?

No, I don't think Proxy can work without a domain

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

678098 · 2025-01-13T17:43:58Z

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

-    QueueLiveState& qinfo = context.d_queueContext_p->d_liveQInfo;
-    const int       pid   = context.d_queueContext_p->partitionId();
+    QueueLiveState& qinfo = context->d_queueContext_p->d_liveQInfo;
+    const int       pid   = context->d_queueContext_p->partitionId();


Here we also dereference context->d_queueContext_p without proper checks

678098 · 2025-01-13T17:46:04Z

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

@@ -1141,12 +1161,12 @@ void ClusterQueueHelper::onOpenQueueResponse(
        failure = false;
    }

-    QueueContext&           qcontext = *context.d_queueContext_p;
+    QueueContext&           qcontext = *context->d_queueContext_p;


Dereferencing

678098 · 2025-01-13T17:47:23Z

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

    QueueLiveState& qinfo        = queueContext->d_liveQInfo;
    const int       pid          = queueContext->partitionId();
    const bool      isPrimary    = !d_cluster_p->isRemote() &&
                           d_clusterState_p->isSelfPrimary(pid);
    bsl::shared_ptr<mqbi::Queue> queue = createQueueFactory(errorDescription,
-                                                            context,
+                                                            *context,


Missing check before that context is not a null shared ptr

src/groups/mqb/mqbblp/mqbblp_domain.cpp

678098 · 2025-01-13T17:51:24Z

src/groups/mqb/mqbblp/mqbblp_domain.h

@@ -89,8 +89,7 @@ namespace mqbblp {
 // ============

 /// Domain implementation
-class Domain BSLS_KEYWORD_FINAL : public mqbi::Domain,
-                                  public mqbc::ClusterStateObserver {
+class Domain : public mqbi::Domain {


Suggested change

class Domain : public mqbi::Domain {

class Domain BSLS_KEYWORD_FINAL : public mqbi::Domain {

src/groups/mqb/mqbi/mqbi_cluster.h

bmq-oss-ci

Build 440 of commit 2f548bf has completed with FAILURE

678098 · 2025-01-15T13:25:14Z

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

+, d_callback(callback)
+{
+    BSLS_ASSERT_SAFE(domain);
+    // NOTHING


Suggested change

// NOTHING

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

678098 · 2025-01-16T13:36:38Z

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

 {
-    QueueLiveState&   info  = context->d_queueContext_p->d_liveQInfo;
+    QueueLiveState&   info  = context->queueContext()->d_liveQInfo;


Might rely on the checks done before calling this function, or might add them here as well

Suggested change

QueueLiveState& info = context->queueContext()->d_liveQInfo;

// executed by the cluster *DISPATCHER* thread

// PRECONDITIONS

BSLS_ASSERT_SAFE(context);

BSLS_ASSERT_SAFE(context->queueContext());

QueueLiveState& info = context->queueContext()->d_liveQInfo;

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

678098 · 2025-01-16T13:44:16Z

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp


-    const int pid = context.d_queueContext_p->partitionId();
+    QueueContext* queueContext = context.queueContext();


Suggested change

QueueContext* queueContext = context.queueContext();

BSLS_ASSERT_SAFE(context);

QueueContext* queueContext = context.queueContext();

BSLS_ASSERT_SAFE(queueContext);

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

dorjesinpo requested a review from a team as a code owner January 13, 2025 15:19

dorjesinpo added the enhancement New feature or request label Jan 13, 2025

dorjesinpo requested review from 678098 and removed request for a team January 13, 2025 15:19

dorjesinpo assigned 678098 Jan 13, 2025

dorjesinpo requested a review from emelialei88 January 13, 2025 15:19

dorjesinpo changed the title ~~CQH calls Queue::getHandle~~ Feat: CQH calls Queue::getHandle Jan 13, 2025

678098 requested changes Jan 13, 2025

View reviewed changes

dorjesinpo force-pushed the dev/streamline-gethandle branch from 6e95521 to 2f548bf Compare January 14, 2025 16:07

bmq-oss-ci bot reviewed Jan 14, 2025

View reviewed changes

678098 requested changes Jan 16, 2025

View reviewed changes

678098 assigned dorjesinpo and unassigned 678098 Jan 17, 2025

dorjesinpo added 3 commits January 23, 2025 10:03

CQH calls Queue::getHandle

fcb052f

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

Asserts...

46fb378

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

more asserts

af5e923

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

dorjesinpo force-pushed the dev/streamline-gethandle branch from 2f548bf to af5e923 Compare January 23, 2025 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: CQH calls Queue::getHandle #565

Feat: CQH calls Queue::getHandle #565

dorjesinpo commented Jan 13, 2025

678098 Jan 13, 2025

dorjesinpo Jan 14, 2025

678098 Jan 13, 2025

678098 Jan 13, 2025

678098 Jan 13, 2025

678098 Jan 13, 2025

bmq-oss-ci bot left a comment

678098 Jan 15, 2025

678098 Jan 16, 2025

678098 Jan 16, 2025

	class Domain : public mqbi::Domain {
	class Domain BSLS_KEYWORD_FINAL : public mqbi::Domain {

-    QueueLiveState&   info  = context->queueContext()->d_liveQInfo;
+    // executed by the cluster *DISPATCHER* thread
+    // PRECONDITIONS
+    BSLS_ASSERT_SAFE(context);
+    BSLS_ASSERT_SAFE(context->queueContext());
+    QueueLiveState&   info  = context->queueContext()->d_liveQInfo;


		const int pid = context.d_queueContext_p->partitionId();
		QueueContext* queueContext = context.queueContext();

Feat: CQH calls Queue::getHandle #565

Are you sure you want to change the base?

Feat: CQH calls Queue::getHandle #565

Conversation

dorjesinpo commented Jan 13, 2025

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

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