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.

Sign up for GitHub

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

Jump to bottom

[C++] Fix send callback might not be invoked in key based batching #14898

Merged
merged 2 commits into from
Mar 28, 2022
Merged

[C++] Fix send callback might not be invoked in key based batching #14898

merged 2 commits into from
Mar 28, 2022

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Mar 27, 2022 •
edited
Loading

Motivation

When C++ client enables key based batching, there is a chance that the
send callback is not invoked. See

pulsar/pulsar-client-cpp/lib/ProducerImpl.cc

Lines 272 to 275 in 32df93f

OpSendMsg opSendMsg;
if (batchMessageContainer_->createOpSendMsg(opSendMsg) == ResultOk) {
callbacks->opSendMsgs.emplace_back(opSendMsg);
}

If a batch container has multiple batches, only one batch could be
processed during closeAsync. Even worse, the semaphores of other
batches won't be released.

Modifications

  • Add a processAndClear method to BatchMessageContainerBase to clear all pending batches and
    process them. Then call this method in closeAsync and
    getPendingCallbacksWhenFailed.
  • Add a test testCloseBeforeSend to verify when a producer has
    multiple pending batches, all callbacks can be invoked in
    closeAsync.

@BewareMyPower
[C++] Fix send callback might not be invoked in key based batching
c3665a9 
### Motivation

When C++ client enables key based batching, there is a chance that the
send callback is not invoked. See
https://github.com/apache/pulsar/blob/32df93f693bfdf42953bd728a12ecdea1796dcc8/pulsar-client-cpp/lib/ProducerImpl.cc#L272-L275

If a batch container has multiple batches, only one batch could be
processed during `closeAsync`. Even worse, the semaphores of other
batches won't be released.

### Modifications

- Add a `clearPendingBatches` method to clear all pending batches and
  process them. Then call this method in `closeAsync` and
  `getPendingCallbacksWhenFailed`.
- Add a test `testCloseBeforeSend` to verify when a producer has
  multiple pending batches, all callbacks can be invoked in
  `closeAsync`.
@BewareMyPower BewareMyPower added type/bug The PR fixed a bug or issue reported a bug component/client-c++ doc-not-needed Your PR changes do not impact docs release/2.9.3 release/2.8.4 release/2.10.1 labels Mar 27, 2022
@BewareMyPower BewareMyPower self-assigned this Mar 27, 2022
@merlimat merlimat added this to the 2.11.0 milestone Mar 28, 2022
@merlimat merlimat merged commit f3295ff into apache:master Mar 28, 2022
@BewareMyPower BewareMyPower deleted the bewaremypower/fix-cpp-key-based-batching branch March 29, 2022 02:09
codelipenghui pushed a commit that referenced this pull request Apr 19, 2022
@BewareMyPower @codelipenghui
[C++] Fix send callback might not be invoked in key based batching (#…
a74f242 
…14898)

* [C++] Fix send callback might not be invoked in key based batching

### Motivation

When C++ client enables key based batching, there is a chance that the
send callback is not invoked. See
https://github.com/apache/pulsar/blob/32df93f693bfdf42953bd728a12ecdea1796dcc8/pulsar-client-cpp/lib/ProducerImpl.cc#L272-L275

If a batch container has multiple batches, only one batch could be
processed during `closeAsync`. Even worse, the semaphores of other
batches won't be released.

### Modifications

- Add a `clearPendingBatches` method to clear all pending batches and
  process them. Then call this method in `closeAsync` and
  `getPendingCallbacksWhenFailed`.
- Add a test `testCloseBeforeSend` to verify when a producer has
  multiple pending batches, all callbacks can be invoked in
  `closeAsync`.

* Add processAndClear() to batch message container

(cherry picked from commit f3295ff)
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
@BewareMyPower
[C++] Fix send callback might not be invoked in key based batching (a…
21db08f 
…pache#14898)

* [C++] Fix send callback might not be invoked in key based batching

### Motivation

When C++ client enables key based batching, there is a chance that the
send callback is not invoked. See
https://github.com/apache/pulsar/blob/32df93f693bfdf42953bd728a12ecdea1796dcc8/pulsar-client-cpp/lib/ProducerImpl.cc#L272-L275

If a batch container has multiple batches, only one batch could be
processed during `closeAsync`. Even worse, the semaphores of other
batches won't be released.

### Modifications

- Add a `clearPendingBatches` method to clear all pending batches and
  process them. Then call this method in `closeAsync` and
  `getPendingCallbacksWhenFailed`.
- Add a test `testCloseBeforeSend` to verify when a producer has
  multiple pending batches, all callbacks can be invoked in
  `closeAsync`.

* Add processAndClear() to batch message container
@mattisonchao
Copy link
Member

@BewareMyPower Would you mind cherry-pick this PR to branch-2.9?

BewareMyPower added a commit that referenced this pull request May 24, 2022
@BewareMyPower
[C++] Fix send callback might not be invoked in key based batching (#…
7134705 
…14898)

* [C++] Fix send callback might not be invoked in key based batching

### Motivation

When C++ client enables key based batching, there is a chance that the
send callback is not invoked. See
https://github.com/apache/pulsar/blob/32df93f693bfdf42953bd728a12ecdea1796dcc8/pulsar-client-cpp/lib/ProducerImpl.cc#L272-L275

If a batch container has multiple batches, only one batch could be
processed during `closeAsync`. Even worse, the semaphores of other
batches won't be released.

### Modifications

- Add a `clearPendingBatches` method to clear all pending batches and
  process them. Then call this method in `closeAsync` and
  `getPendingCallbacksWhenFailed`.
- Add a test `testCloseBeforeSend` to verify when a producer has
  multiple pending batches, all callbacks can be invoked in
  `closeAsync`.

* Add processAndClear() to batch message container

(cherry picked from commit f3295ff)
@BewareMyPower BewareMyPower added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label May 24, 2022
@BewareMyPower BewareMyPower mentioned this pull request May 24, 2022
Merged
BewareMyPower added a commit that referenced this pull request Jul 27, 2022
@BewareMyPower
[C++] Fix send callback might not be invoked in key based batching (#…
f49047d 
…14898)

* [C++] Fix send callback might not be invoked in key based batching

### Motivation

When C++ client enables key based batching, there is a chance that the
send callback is not invoked. See
https://github.com/apache/pulsar/blob/32df93f693bfdf42953bd728a12ecdea1796dcc8/pulsar-client-cpp/lib/ProducerImpl.cc#L272-L275

If a batch container has multiple batches, only one batch could be
processed during `closeAsync`. Even worse, the semaphores of other
batches won't be released.

### Modifications

- Add a `clearPendingBatches` method to clear all pending batches and
  process them. Then call this method in `closeAsync` and
  `getPendingCallbacksWhenFailed`.
- Add a test `testCloseBeforeSend` to verify when a producer has
  multiple pending batches, all callbacks can be invoked in
  `closeAsync`.

* Add processAndClear() to batch message container

(cherry picked from commit f3295ff)
@BewareMyPower BewareMyPower added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merlimat merlimat merlimat approved these changes

@aahmed-se aahmed-se Awaiting requested review from aahmed-se

@jiazhai jiazhai Awaiting requested review from jiazhai

@massakam massakam Awaiting requested review from massakam

@rdhabalia rdhabalia Awaiting requested review from rdhabalia

Assignees

@BewareMyPower BewareMyPower

Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.8.4 release/2.9.3 release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@BewareMyPower @mattisonchao @merlimat @codelipenghui