You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/04/06 05:14:36 UTC
[GitHub] [pulsar] merlimat opened a new pull request #10144: [C++] Fixed releasing semaphore and memory quota after send timeout
merlimat opened a new pull request #10144:
URL: https://github.com/apache/pulsar/pull/10144
### Motivation
C++ client is not releasing the semaphore and reserved memory correctly in case of send timeouts. This is a regression introduced in #9676, since the previous code was using the queue itself for accounting for the available spots (hence: emptying the queue was enough).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #10144: [C++] Fixed releasing semaphore and memory quota after send timeout
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #10144:
URL: https://github.com/apache/pulsar/pull/10144#discussion_r607996099
##########
File path: pulsar-client-cpp/lib/ProducerImpl.cc
##########
@@ -255,12 +255,14 @@ std::shared_ptr<ProducerImpl::PendingCallbacks> ProducerImpl::getPendingCallback
// without holding producer mutex.
for (auto& op : pendingMessagesQueue_) {
callbacks->opSendMsgs.push_back(op);
+ releaseSemaphoreForSendOp(op);
}
if (batchMessageContainer_) {
OpSendMsg opSendMsg;
if (batchMessageContainer_->createOpSendMsg(opSendMsg) == ResultOk) {
callbacks->opSendMsgs.emplace_back(opSendMsg);
+ releaseSemaphoreForSendOp(opSendMsg);
Review comment:
That's 100% correct again. The opSendMsg is always set by the `batchMessageContainer`. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng merged pull request #10144: [C++] Fixed releasing semaphore and memory quota after send timeout
Posted by GitBox <gi...@apache.org>.
jerrypeng merged pull request #10144:
URL: https://github.com/apache/pulsar/pull/10144
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] erobot commented on a change in pull request #10144: [C++] Fixed releasing semaphore and memory quota after send timeout
Posted by GitBox <gi...@apache.org>.
erobot commented on a change in pull request #10144:
URL: https://github.com/apache/pulsar/pull/10144#discussion_r607837748
##########
File path: pulsar-client-cpp/lib/ProducerImpl.cc
##########
@@ -255,12 +255,14 @@ std::shared_ptr<ProducerImpl::PendingCallbacks> ProducerImpl::getPendingCallback
// without holding producer mutex.
for (auto& op : pendingMessagesQueue_) {
callbacks->opSendMsgs.push_back(op);
+ releaseSemaphoreForSendOp(op);
}
if (batchMessageContainer_) {
OpSendMsg opSendMsg;
if (batchMessageContainer_->createOpSendMsg(opSendMsg) == ResultOk) {
callbacks->opSendMsgs.emplace_back(opSendMsg);
+ releaseSemaphoreForSendOp(opSendMsg);
Review comment:
The memory is already reserved, so I think releaseSemaphoreForSendOp should be called no matter batchMessageContainer_->createOpSendMsg() returns ok or not. (But this relys on that batchMessageContainer_->createOpSendMsg() sets opSendMsg correctly even if it returns failed. Although it does seem true this time.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org