Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Getting rid of (un)assigningCb #507

Merged
merged 4 commits into from
Dec 10, 2024
Merged

refactor: Getting rid of (un)assigningCb #507

merged 4 commits into from
Dec 10, 2024

Conversation

dorjesinpo
Copy link
Collaborator

@dorjesinpo dorjesinpo commented Nov 8, 2024

One more step converging CSL/non-CSL

@dorjesinpo dorjesinpo requested a review from a team as a code owner November 8, 2024 00:02
@dorjesinpo dorjesinpo force-pushed the fix/squeaky-3 branch 3 times, most recently from 34db1aa to d4ab844 Compare November 10, 2024 17:31
@dorjesinpo dorjesinpo changed the title WIP Getting rid of (un)assigningCb Nov 11, 2024
@dorjesinpo dorjesinpo changed the title Getting rid of (un)assigningCb refactor: Getting rid of (un)assigningCb Nov 11, 2024
@dorjesinpo dorjesinpo requested a review from kaikulimu November 11, 2024 19:55
Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build 371 of commit 547f190 has completed with FAILURE

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build 374 of commit f2eb61b has completed with FAILURE

// Set the queue as assigning (no longer pending unassignment)

UriToQueueInfoMapCIter qcit = domIt->second->queuesInfo().find(uri);
if (qcit != domIt->second->queuesInfo().cend()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach doesn't actually work. Assignment is a different story than unassignment. During first pending assignment, the queue does not exist in ClusterState yet, so qcit won't find anything, and thus won't set anything to k_ASSIGNING.

Lemma: If the queue exists in ClusterState, then it is never in k_ASSIGNING state.

Therefore, we need a separate collection to keep track of k_ASSIGNING queues, and once assignment at ClusterState completes, we need to remove the corresponding entry.

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build 387 of commit ea72c8e has completed with FAILURE

@kaikulimu
Copy link
Collaborator

kaikulimu commented Nov 26, 2024

Some notes for myself

  • ClusterQueueHelper::onQueueAssigned:

    • Legacy mode: Same observer callback, but skipped due to immediate return
    • FSM mode: Observer callback of ClusterState::assignQueue
  • ClusterQueueHelper::onQueueAssigning:

    • Legacy mode: Called during ClusterStateManager::assignQueue, ClusterStateManager::registerQueueInfo, ClusterStateManager::processQueueAssignmentRequest, ClusterStateManager::processQueueAssignmentAdvisory
    • FSM mode: Called during ClusterStateManager::assignQueue, ClusterStateManager::registerQueueInfo, ClusterStateManager::processQueueAssignmentRequest

    Both modes share the same first three codepaths via ClusterUtil
    All above methods eventually forwards to ClusterUtil::assignQueue

  • ClusterQueueHelper::onQueueUnassigned:

    • Legacy mode: Same observer callback, but skipped due to immediate return
    • FSM mode: Observer callback of ClusterState::unassignQueue
  • ClusterQueueHelper::onQueueUnassigning:

    In legacy mode, on the leader, removeQueueRaw is called during ClusterQueueHelper::gcExpiredQueues

Places where ClusterQueueHelper::QueueContextMap d_queues are inserted to:

  1. ClusterQueueHelper::onQueueAssigned
  2. ClusterQueueHelper::openQueue
  3. ClusterQueueHelper::onQueueAssigning

If a brand new queue is being opened, ClusterQueueHelper::openQueue will create a new QueueContext. Then, the queue assignment flow will use this context instead of creating its own.

@kaikulimu
Copy link
Collaborator

Reviewing, should be done by Dev 5

@kaikulimu
Copy link
Collaborator

Progress update:
Queue unassignment flow has been reviewed.
Now moving on to queue assignment flow.

I have refrained from posting my review, in case my perception on queue unassignment aspects changes after I review queue assignment flow.

Copy link
Collaborator

@kaikulimu kaikulimu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review done. Thanks for the hard work! This part of the code is not easy to navigate.

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.h Outdated Show resolved Hide resolved
src/groups/mqb/mqbc/mqbc_clusterstate.h Show resolved Hide resolved
src/groups/mqb/mqbc/mqbc_clusterstate.h Outdated Show resolved Hide resolved
src/groups/mqb/mqbc/mqbc_clusterstate.h Outdated Show resolved Hide resolved
src/groups/mqb/mqbc/mqbc_clusterstate.h Outdated Show resolved Hide resolved
src/groups/mqb/mqbc/mqbc_clusterutil.h Show resolved Hide resolved
src/groups/mqb/mqbc/mqbc_clusterutil.cpp Show resolved Hide resolved
src/groups/mqb/mqbc/mqbc_clusterutil.cpp Show resolved Hide resolved
src/groups/mqb/mqbblp/mqbblp_clusterstatemanager.cpp Outdated Show resolved Hide resolved
src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp Outdated Show resolved Hide resolved
@kaikulimu kaikulimu assigned dorjesinpo and unassigned kaikulimu Dec 6, 2024
Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build 404 of commit 263600c has completed with FAILURE

Copy link
Collaborator

@kaikulimu kaikulimu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kaikulimu kaikulimu merged commit acf74d3 into main Dec 10, 2024
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants