Skip to content

Fix[mqb]: revisiting open queue failures handling - #1175

Merged
dorjesinpo merged 4 commits into
mainfrom
fix/handling-open-failure
Mar 31, 2026
Merged

Fix[mqb]: revisiting open queue failures handling#1175
dorjesinpo merged 4 commits into
mainfrom
fix/handling-open-failure

Conversation

@dorjesinpo

@dorjesinpo dorjesinpo commented Mar 3, 2026

Copy link
Copy Markdown
Collaborator

Upon open queue failure, either buffer the request or retry immediately, or respond

@dorjesinpo
dorjesinpo requested a review from a team as a code owner March 3, 2026 20:36
@dorjesinpo dorjesinpo added the bug Something isn't working label Mar 3, 2026
@dorjesinpo
dorjesinpo requested a review from 678098 March 5, 2026 21:01
@dorjesinpo
dorjesinpo force-pushed the fix/handling-open-failure branch 2 times, most recently from aea57d5 to eca0625 Compare March 12, 2026 14:03
Comment on lines +1450 to +1453
// Do retry if the state is k_OPEN && isQueuePrimaryAvailable
bool retryNow = subQueueContext.d_state == SubQueueContext::k_OPEN
? isQueuePrimaryAvailable(*qcontext, otherThan)
: false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Do retry if the state is k_OPEN && isQueuePrimaryAvailable
bool retryNow = subQueueContext.d_state == SubQueueContext::k_OPEN
? isQueuePrimaryAvailable(*qcontext, otherThan)
: false;
bool retryNow = (subQueueContext.d_state == SubQueueContext::k_OPEN)
&& isQueuePrimaryAvailable(*qcontext, otherThan);

Possible to replace ternary operator with operations mentioned in the comment.
This way, the code becomes self-explanatory, and the comment duplicating behaviour can be removed.

@dorjesinpo
dorjesinpo force-pushed the fix/handling-open-failure branch 3 times, most recently from 2baa5da to e2eb738 Compare March 24, 2026 01:52
@dorjesinpo dorjesinpo assigned dorjesinpo and unassigned 678098 Mar 24, 2026
@dorjesinpo
dorjesinpo force-pushed the fix/handling-open-failure branch from e2eb738 to 92bb647 Compare March 24, 2026 21:05
@dorjesinpo
dorjesinpo force-pushed the fix/handling-open-failure branch from 92bb647 to b00775c Compare March 25, 2026 18:32
Comment thread src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp Outdated
Comment thread src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp Outdated
Comment thread src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp Outdated
for (ReopenCycles::const_iterator cit = d_reopenCycles.begin();
cit != d_reopenCycles.end();
++cit) {
sum += cit->second.use_count();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

To be precise, use_count only shows the number of shared_ptr copies, not the number of pending requests. This might be misleading during debug.

For example, there might be 1 shared_ptr with 3 copies, numPendingReopenQueueRequests() == 3 which is not true. We might spend time investigating "where are these 3 reopen requests?", when in reality we only have 1.

Binary +1 or +0 makes more sense here:

Suggested change
sum += cit->second.use_count();
sum += static_cast<int>(cit->second.use_count() > 0);

or

Suggested change
sum += cit->second.use_count();
sum += static_cast<int>(!cit->second.expired());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Using cit->second.use_count() > 0) would return number of partitions being reopened.
The use_count can have the error margin of +1 - upon reopen response we may bind the cycle to reconfigure or retry callback before releasing. It is like having two requests for the same queue - one is completing and another is starting.
But. Since numPendingReopenQueueRequests() is executed in the cluster thread - where we process reopen responses - the "completing" count should be gone and the number should be precise.

Comment thread src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.h Outdated
Comment thread src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.h Outdated
Comment thread src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.h Outdated
Comment thread src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.h
BSLS_ASSERT_SAFE(cycle);

if (cycle->isSuccess()) {
d_reopenCycles.erase(cycle->partitionId());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that we should also check that the current d_reopenCycle[partitionId] has the same primary lease id as cycle before deleting.
What if we free the last PartitionReopenCycle from the previous primary at the time when we have a new primary and another partition cycle for the same partition id?

@dorjesinpo dorjesinpo Mar 30, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This runs in the cluster thread only. We setAsFailure if generationCounts mismatch). Not sure about checking generationCount - 1) there is no shared_ptr anymore; 2) the actual ClusterStatePartitionInfo::primaryLeaseId() could change before we start new cycle.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If it runs on cluster thread, let's add a comment

// Thread: CLUSTER dispatcher

And add a precondition

BSLS_ASSERT_SAFE(d_cluster_p->inDispatcherThread());

@dorjesinpo
dorjesinpo force-pushed the fix/handling-open-failure branch from b00775c to 361c84e Compare March 30, 2026 15:06
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
@dorjesinpo
dorjesinpo force-pushed the fix/handling-open-failure branch from 361c84e to 6bb2388 Compare March 30, 2026 17:41
@dorjesinpo dorjesinpo assigned 678098 and unassigned dorjesinpo Mar 30, 2026
Comment on lines +1242 to +1244
cycle.reset(new (*d_allocator_p)
PartitionReopenCycle(this, generationCount, partitionId),
d_allocator_p);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cycle.reset(new (*d_allocator_p)
PartitionReopenCycle(this, generationCount, partitionId),
d_allocator_p);
cycle = bsl::allocate_shared<PartitionReopenCycle>(d_allocator_p, this, generationCount, partitionId);

@678098 678098 assigned dorjesinpo and unassigned 678098 Mar 30, 2026
@dorjesinpo
dorjesinpo merged commit a4cb800 into main Mar 31, 2026
48 checks passed
@dorjesinpo
dorjesinpo deleted the fix/handling-open-failure branch March 31, 2026 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants