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

Wait ACKs for all messages posted by bmqtool #231

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

syuzvinsky
Copy link
Collaborator

Instead of waiting for Ctrl+C in auto-posting mode, bmqtool will be counting ACKs and terminate after all posted messages are acknowledged.

@syuzvinsky syuzvinsky requested a review from a team as a code owner March 29, 2024 15:10
@syuzvinsky syuzvinsky force-pushed the count-acks branch 6 times, most recently from 7d34985 to 9d25ce5 Compare March 29, 2024 17:07
@syuzvinsky syuzvinsky marked this pull request as draft March 31, 2024 19:32
@syuzvinsky syuzvinsky marked this pull request as ready for review April 2, 2024 14:12
// messages, after which the shutdown semaphore will
// be posted.

int d_numPostedAcknowledged;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose to use 64 bit integer type instead for both counters, because it is possible to have int32 overflow here, especially during soak testing.

With achievable rate of 100k messages/second it will take approx 6 hours ((2**31) / 100000 / 3600 ~= 5.965) to overflow. If you reduce this stress rate by a multiple of 10 for soak testing (10k/s), it will take 60 hours to overflow which is still achievable.

Even the signed bsls::Types::Int64 should be enough here, because in practise overflow will not be achievable (almost 3 million years to overflow with signed int64 type).

Copy link
Collaborator Author

@syuzvinsky syuzvinsky Apr 3, 2024

Choose a reason for hiding this comment

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

Good catch in general!
Changing to bsl::uint64_t though, because:

  1. I don't plan to store negative values there, so unsigned type is chosen;
  2. ::uint64_t did not seem to be supported by C++03;
  3. bsls::Types::UInt64 is a legacy type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think bsl::uint64_t is a good choice, if you don't want to add a special meaning for negative values here.

Note that for bmqbrkr and related libs we preferred to use bsls::Types::... at least while we support AIX/Solaris, using bsl::uint64_t in bmqtool should be fine though

if (bmqt::QueueFlagsUtil::isWriter(d_parameters_p->queueFlags())) {
d_numExpectedAcks = d_parameters_p->eventsCount() *
d_parameters_p->eventSize();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's worth to change these params types to xs:long in xsd, so the product here is less prone to overflow with "seemingly not too big" param values.

Copy link
Collaborator

@678098 678098 Apr 2, 2024

Choose a reason for hiding this comment

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

At the same time, I am not sure if we should add more constraints for param values, like:

BSLS_ASSERT(eventsCount() >= 0);
BSLS_ASSERT(eventsCount() <= 2^31);
BSLS_ASSERT(eventSize() >= 1);
BSLS_ASSERT(eventSize() <= 2^31);

Another way, these constraints might be introduced in xsd if possible.

@syuzvinsky syuzvinsky requested a review from dorjesinpo April 3, 2024 15:21
Copy link
Collaborator

@dorjesinpo dorjesinpo left a comment

Choose a reason for hiding this comment

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

some questions

// messages, after which the shutdown semaphore will
// be posted.

int d_numPostedAcknowledged;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this 'posted'? It looks like an ACK counter, regardless of what is posted. Or does it correlate GUIDs to make sure it is a valid ACK?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it does not correlate ACKs, but is there anything which can be ACKed other than posted messages?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is nothing. So, the "posted' part does not carry much meaning, does it. It is just a bit confusing because one can think we have posted ACKs.
Very minor

d_parameters_p->shutdownGrace() != 0) {
// We do not need to sleep the grace period, since it is done
// by the main thread, in the stop() function.
if (!bmqt::QueueFlagsUtil::isAck(d_parameters_p->queueFlags())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you verify the use of shutdownGrace now? Do we still need it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will double check this with Jean-Louis and update the response after that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed this with Julien Bibollet.
The conclusion was that shutdownGrace was designed, and is still needed in consuming mode: some integration tests imply running several instances of the consumer, and there is some nondeterminism in how many messages each particular instance should receive, only the total number of messages is defined.
So, the test code will be waiting for a publisher to complete, and after that it will be waiting until each client stops receiving messages and terminate.
In the mean time, using shutdownGrace in a publisher thread looks rather like a hack, and counting ACKs is a better way to make sure that all messages have reached the cluster.

@syuzvinsky syuzvinsky force-pushed the count-acks branch 3 times, most recently from 71305e1 to 9d0548a Compare April 5, 2024 14:20
Instead of waiting for Ctrl+C in auto-posting mode, bmqtool will be
counting ACKs and terminate after all posted messages are acknowledged.

Signed-off-by: Stanislav Yuzvinsky <syuzvinsky@bloomberg.net>
Signed-off-by: Stanislav Yuzvinsky <syuzvinsky@bloomberg.net>
@syuzvinsky syuzvinsky merged commit 18b9adc into bloomberg:main Apr 8, 2024
10 checks passed
@syuzvinsky syuzvinsky deleted the count-acks branch April 8, 2024 10:53
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
Instead of waiting for Ctrl+C in auto-posting mode, bmqtool will be
counting ACKs and terminate after all posted messages are acknowledged.

Signed-off-by: Stanislav Yuzvinsky <syuzvinsky@bloomberg.net>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
Instead of waiting for Ctrl+C in auto-posting mode, bmqtool will be
counting ACKs and terminate after all posted messages are acknowledged.

Signed-off-by: Stanislav Yuzvinsky <syuzvinsky@bloomberg.net>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
Instead of waiting for Ctrl+C in auto-posting mode, bmqtool will be
counting ACKs and terminate after all posted messages are acknowledged.

Signed-off-by: Stanislav Yuzvinsky <syuzvinsky@bloomberg.net>
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.

3 participants