-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[improve][cpp] Support acknowledging a list of messages #17739
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
Conversation
|
Could you rebase to master? I saw some strange errors in CI. |
07f5b35 to
1cf0c58
Compare
I rebased master, PTAL. |
| this->ackGroupingTrackerPtr_->addAcknowledgeList(messageIdList); | ||
| for (const auto& messageId : messageIdList) { | ||
| this->unAckedMessageTrackerPtr_->remove(messageId); | ||
| this->batchAcknowledgementTracker_.deleteAckedMessage(messageId, proto::CommandAck::Individual); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add thread safe remove and deleteAckedMessage methods that accept MessageIdList to make these calls thread safe. You can see both UnAckedMessageTrackerEnabled::remove and BatchAcknowledgementTracker::deleteAckedMessage are thread safe so the acknowledgeAsync method for a single message won't lead to a race condition for each component (unAckedMessageTrackerPtr_ and unAckedMessageTrackerPtr_)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, It is better to provide an accept MessageIdList method. I will provide it.
But I want to know what is the race condition for the actual scenario. For UnAckedMessageTracker, Will there be the following scenario?
have a MessageIdList: {1, 2, 3, 4}
on acknowledge thread:
for MessageIdList{
this->unAckedMessageTrackerPtr_->remove(msgId);
}and another thread to call: this->unAckedMessageTrackerPtr_->add(msgId);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I add new remove(MessageIdList) method on UnAckedMessageTracker, Since BatchAcknowledgementTracker not support Individual ack, so, I remove invoke this->batchAcknowledgementTracker_.deleteAckedMessage code. PTAL. Thanks.
|
Resubmit the PR in pulsar-client-cpp repository: apache/pulsar-client-cpp#23 |
Motivation
apache/pulsar-client-cpp#90
Modifications
Documentation
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)
Matching PR in forked repository
shibd#13