Feat: removing shutdown v1 #784
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is exciting. |
dorjesinpo
force-pushed
the
dev/remove-shutdown-v1
branch
from
9c5c451 to
ed55a70
Compare
Signed-off-by: dorjesinpo <[email protected]>
dorjesinpo
force-pushed
the
dev/remove-shutdown-v1
branch
from
ed55a70 to
5a135bd
Compare
![bmq-oss-ci[bot]](https://avatars.githubusercontent.com/u/1416818?s=60&v=4){kind=small}
678098
reviewed
Jun 17, 2025
| BMQTST_ASSERT_EQ(callbackCounter, 1); | ||
| } | ||
|
|
||
| PV("Shutdown with unconfirmed messages and timeout"); |
There was a problem hiding this comment.
because ClientSession is not in the business of waiting for unconfirmed messages
| initiateShutdown(const VoidFunctor& callback, | ||
| bool supportShutdownV2 = false) BSLS_KEYWORD_OVERRIDE; | ||
| /// sequence. Execute shutdown logic where upstream (not downstream) nodes | ||
| /// deconfigure queues and the shutting down node waits for CONFIRMS. |
There was a problem hiding this comment.
Suggested change
| /// deconfigure queues and the shutting down node waits for CONFIRMS. | |
| /// deconfigure queues and the shutting down node waits for CONFIRMS. |
| << ": invalid cluster name in the StopRequest from " | ||
| << source->nodeDescription() << ", " << request; | ||
| return; // RETURN | ||
| } |
There was a problem hiding this comment.
Do you think it's worth adding a warning and early return if someone sends bmqp::Protocol::eStopRequestVersion::e_V1 here?
There was a problem hiding this comment.
good question. this PR is based on the assumption of never having v1. At some point, we need to retire stopRequest.version.
Comment on lines
186
to
187
| // Add callback to be invoked once V1 shuts down all client sessions or | ||
| // V2 finishes waiting for unconfirmed |
There was a problem hiding this comment.
Suggested change
| // Add callback to be invoked once V1 shuts down all client sessions or | |
| // V2 finishes waiting for unconfirmed | |
| // Add callback to be invoked once V2 finishes waiting for unconfirmed |
| initiateShutdown(const VoidFunctor& callback, | ||
| bool supportShutdownV2 = false) BSLS_KEYWORD_OVERRIDE; | ||
| /// invoked. Execute shutdown logic where upstream (not downstream) nodes | ||
| /// deconfigure queues and the shutting down node waits for CONFIRMS. |
There was a problem hiding this comment.
Suggested change
| /// deconfigure queues and the shutting down node waits for CONFIRMS. | |
| /// deconfigure queues and the shutting down node waits for CONFIRMS. |
| @@ -321,8 +321,7 @@ int Cluster::start(BSLA_UNUSED bsl::ostream& errorDescription) | |||
| return 0; | |||
| } | |||
|
|
|||
| void Cluster::initiateShutdown(BSLA_UNUSED const VoidFunctor& callback, | |||
| BSLA_UNUSED bool supportShutdownV2) | |||
| void Cluster::initiateShutdown(BSLA_UNUSED const VoidFunctor& callback) | |||
| { | |||
| // PRECONDITIONS | |||
| BSLS_ASSERT_OPT(!d_isStarted && | |||
There was a problem hiding this comment.
The condition here looks odd, and the comment start() can only be called once on this object is irrelevant to shutdown.
Do we set d_isStarted = false before we call Cluster::initiateShutdown?
There was a problem hiding this comment.
This seems like an (old) bug. We never call mqbmock::Cluster::initiateShutdown
We can change this to
BSLS_ASSERT_OPT(d_isStarted &&
"start() must be called before initiateShutdown")
| bsls::Types::Int64 | ||
| countUnconfirmed(unsigned int subId) BSLS_KEYWORD_OVERRIDE; | ||
| /// Return number of unconfirmed messages across all handles. | ||
| bsls::Types::Int64 countUnconfirmed() BSLS_KEYWORD_OVERRIDE; |
There was a problem hiding this comment.
Suggested change
| bsls::Types::Int64 countUnconfirmed() BSLS_KEYWORD_OVERRIDE; | |
| bsls::Types::Int64 countUnconfirmed() const BSLS_KEYWORD_OVERRIDE; |
| @@ -245,7 +245,7 @@ Queue::setStats(const bsl::shared_ptr<mqbstat::QueueStatsDomain>& stats) | |||
| d_stats_sp = stats; | |||
| } | |||
|
|
|||
| bsls::Types::Int64 Queue::countUnconfirmed(BSLA_UNUSED unsigned int subId) | |||
| bsls::Types::Int64 Queue::countUnconfirmed() | |||
There was a problem hiding this comment.
Suggested change
| bsls::Types::Int64 Queue::countUnconfirmed() | |
| bsls::Types::Int64 Queue::countUnconfirmed() const |
| BSLA_UNUSED const ShutdownCb& callback, | ||
| BSLA_UNUSED const bsls::TimeInterval& timeout, | ||
| BSLA_UNUSED bool supportShutdownV2) | ||
| void DummySession::initiateShutdown(const ShutdownCb& callback) |
There was a problem hiding this comment.
Suggested change
| void DummySession::initiateShutdown(const ShutdownCb& callback) | |
| void DummySession::initiateShutdown(BSLA_UNUSED const ShutdownCb& callback) |
| void initiateShutdown(const ShutdownCb& callback, | ||
| const bsls::TimeInterval& timeout, | ||
| bool supportShutdownV2 = false) BSLS_KEYWORD_OVERRIDE | ||
| void initiateShutdown(const ShutdownCb& callback) BSLS_KEYWORD_OVERRIDE |
There was a problem hiding this comment.
Suggested change
| void initiateShutdown(const ShutdownCb& callback) BSLS_KEYWORD_OVERRIDE | |
| void initiateShutdown(BSLA_UNUSED const ShutdownCb& callback) BSLS_KEYWORD_OVERRIDE |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is intended to remove the previous shutdown logic assuming shutdown v2 is everywhere