-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[C++] Fix message id is always the default value in send callback #6812
Conversation
/pulsarbot run cpp-tests |
/pulsarbot run unit-tests |
ASSERT_EQ(ResultOk, result); | ||
ASSERT_EQ(i, id.batchIndex()); | ||
sentIdSet.emplace(id); |
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.
There's no hard guarantee that this callback will be executed before the receive loop is done.
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.
Thank you for pointing out this! I missed the point that the SendReceipt
to producer is not guaranteed to be completed before the consumer's receive()
done. I'll do some work on it.
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.
There is a Latch
class in lib/
that might be helpful here.
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.
Thanks! I found the Latch
just now :)
/pulsarbot run-failure-checks |
1 similar comment
/pulsarbot run-failure-checks |
…ache#6812) * Fix bug: sendCallback's 2nd argument was always the default MessageId * Set batch index for each message's callback of batch * Add test for message id in send callback * Ensure all send callbacks completed before ASSERT_EQ
…ache#6812) * Fix bug: sendCallback's 2nd argument was always the default MessageId * Set batch index for each message's callback of batch * Add test for message id in send callback * Ensure all send callbacks completed before ASSERT_EQ
…ache#6812) * Fix bug: sendCallback's 2nd argument was always the default MessageId * Set batch index for each message's callback of batch * Add test for message id in send callback * Ensure all send callbacks completed before ASSERT_EQ
) * Fix bug: sendCallback's 2nd argument was always the default MessageId * Set batch index for each message's callback of batch * Add test for message id in send callback * Ensure all send callbacks completed before ASSERT_EQ (cherry picked from commit fc69a62)
…ache#6812) * Fix bug: sendCallback's 2nd argument was always the default MessageId * Set batch index for each message's callback of batch * Add test for message id in send callback * Ensure all send callbacks completed before ASSERT_EQ
…ache#6812) * Fix bug: sendCallback's 2nd argument was always the default MessageId * Set batch index for each message's callback of batch * Add test for message id in send callback * Ensure all send callbacks completed before ASSERT_EQ
Motivation
After commit of #4817, the send callback's 2nd argument became
MessageId
, but theMessageId
in callback is always the default value(-1, -1, -1, -1)
. We need the message id in send callback if messages were sent successfully.The problem is that the correct message id has been retrieved in
ProducerImpl::ackReceived
but not passed to the user provided callback. Because after messages were sent toBatchMessageContainer
, the wrapper of user provided callback used theMessageId
of user constructedMessage
as the 2nd argument, which is always(-1, -1, -1, -1)
.Modifications
messageId
ofBatchMessageContainer::MessageContainer
. Then addconst MessageId&
argument tobatchMessageCallBack
instead.Verifying this change
This change added tests and can be verified as follows:
Run
BasicEndToEndTest.testSendCallback
andBatchMessageTest.testSendCallback
.