[C++] Fix UnknownError might be returned for a partitioned producer #15161
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes apache#15078 ### Motivation When a partitioned producer is created and some of the partitioned failed to create, `closeAsync` will be called immediately, even if other partitions were still in progress of creating the associated single producers. Since `closeAsync` is called before calling `setFailed` on the `partitionedProducerCreatedPromise_` field, there is a race condition that all single producers are closed before the promise is set. Then the promise will be set with `ResultUnknownError`, see https://github.com/apache/pulsar/blob/4aeeed5dab9dfe9493526f36d539b3ef29cf6fe5/pulsar-client-cpp/lib/PartitionedProducerImpl.cc#L317. ### Modifications Only after all single producers failed or succeeded then call `closeAsync` if one of them failed. And ensure `partitionedProducerCreatedPromise_` was completed before calling `closeAsync`. This PR also makes the state of a partitioned producer atomic because using a mutex to protect it makes code hard to write. ### Verifying this change Create a separate namespace `public/test-backlog-quotas` to test the case when the backlog quota exceeds. Then add `testBacklogQuotasExceeded` test to make some backlog via creating a consumer and sending some messages to a partition of the topic. In this test, only 1 partition has backlog and will fail with the related error. So the test verifies that `createProducer` could return a correct error instead of `ResultUnknownError`.
BewareMyPower
added
type/bug
BewareMyPower
requested review from
merlimat,
rdhabalia,
jiazhai,
aahmed-se and
massakam
|
@BewareMyPower:Thanks for your contribution. For this PR, do we need to update docs? |
|
@BewareMyPower is this a bug fix and we do not need to update docs? |
BewareMyPower
added
doc-not-needed
|
@BewareMyPower:Thanks for your contribution. For this PR, do we need to update docs? |
BewareMyPower
added
doc-not-needed
aparajita89
pushed a commit
to aparajita89/pulsar
that referenced
this pull request
Apr 18, 2022
…pache#15161) Fixes apache#15078 ### Motivation When a partitioned producer is created and some of the partitioned failed to create, `closeAsync` will be called immediately, even if other partitions were still in progress of creating the associated single producers. Since `closeAsync` is called before calling `setFailed` on the `partitionedProducerCreatedPromise_` field, there is a race condition that all single producers are closed before the promise is set. Then the promise will be set with `ResultUnknownError`, see https://github.com/apache/pulsar/blob/4aeeed5dab9dfe9493526f36d539b3ef29cf6fe5/pulsar-client-cpp/lib/PartitionedProducerImpl.cc#L317. ### Modifications Only after all single producers failed or succeeded then call `closeAsync` if one of them failed. And ensure `partitionedProducerCreatedPromise_` was completed before calling `closeAsync`. This PR also makes the state of a partitioned producer atomic because using a mutex to protect it makes code hard to write. ### Verifying this change Create a separate namespace `public/test-backlog-quotas` to test the case when the backlog quota exceeds. Then add `testBacklogQuotasExceeded` test to make some backlog via creating a consumer and sending some messages to a partition of the topic. In this test, only 1 partition has backlog and will fail with the related error. So the test verifies that `createProducer` could return a correct error instead of `ResultUnknownError`.
codelipenghui
pushed a commit
that referenced
this pull request
Apr 19, 2022
…15161) Fixes #15078 ### Motivation When a partitioned producer is created and some of the partitioned failed to create, `closeAsync` will be called immediately, even if other partitions were still in progress of creating the associated single producers. Since `closeAsync` is called before calling `setFailed` on the `partitionedProducerCreatedPromise_` field, there is a race condition that all single producers are closed before the promise is set. Then the promise will be set with `ResultUnknownError`, see https://github.com/apache/pulsar/blob/4aeeed5dab9dfe9493526f36d539b3ef29cf6fe5/pulsar-client-cpp/lib/PartitionedProducerImpl.cc#L317. ### Modifications Only after all single producers failed or succeeded then call `closeAsync` if one of them failed. And ensure `partitionedProducerCreatedPromise_` was completed before calling `closeAsync`. This PR also makes the state of a partitioned producer atomic because using a mutex to protect it makes code hard to write. ### Verifying this change Create a separate namespace `public/test-backlog-quotas` to test the case when the backlog quota exceeds. Then add `testBacklogQuotasExceeded` test to make some backlog via creating a consumer and sending some messages to a partition of the topic. In this test, only 1 partition has backlog and will fail with the related error. So the test verifies that `createProducer` could return a correct error instead of `ResultUnknownError`. (cherry picked from commit 0f85596)
codelipenghui
pushed a commit
that referenced
this pull request
Apr 19, 2022
…15161) Fixes #15078 When a partitioned producer is created and some of the partitioned failed to create, `closeAsync` will be called immediately, even if other partitions were still in progress of creating the associated single producers. Since `closeAsync` is called before calling `setFailed` on the `partitionedProducerCreatedPromise_` field, there is a race condition that all single producers are closed before the promise is set. Then the promise will be set with `ResultUnknownError`, see https://github.com/apache/pulsar/blob/4aeeed5dab9dfe9493526f36d539b3ef29cf6fe5/pulsar-client-cpp/lib/PartitionedProducerImpl.cc#L317. Only after all single producers failed or succeeded then call `closeAsync` if one of them failed. And ensure `partitionedProducerCreatedPromise_` was completed before calling `closeAsync`. This PR also makes the state of a partitioned producer atomic because using a mutex to protect it makes code hard to write. Create a separate namespace `public/test-backlog-quotas` to test the case when the backlog quota exceeds. Then add `testBacklogQuotasExceeded` test to make some backlog via creating a consumer and sending some messages to a partition of the topic. In this test, only 1 partition has backlog and will fail with the related error. So the test verifies that `createProducer` could return a correct error instead of `ResultUnknownError`. (cherry picked from commit 0f85596)
Nicklee007
pushed a commit
to Nicklee007/pulsar
that referenced
this pull request
Apr 20, 2022
…pache#15161) Fixes apache#15078 ### Motivation When a partitioned producer is created and some of the partitioned failed to create, `closeAsync` will be called immediately, even if other partitions were still in progress of creating the associated single producers. Since `closeAsync` is called before calling `setFailed` on the `partitionedProducerCreatedPromise_` field, there is a race condition that all single producers are closed before the promise is set. Then the promise will be set with `ResultUnknownError`, see https://github.com/apache/pulsar/blob/4aeeed5dab9dfe9493526f36d539b3ef29cf6fe5/pulsar-client-cpp/lib/PartitionedProducerImpl.cc#L317. ### Modifications Only after all single producers failed or succeeded then call `closeAsync` if one of them failed. And ensure `partitionedProducerCreatedPromise_` was completed before calling `closeAsync`. This PR also makes the state of a partitioned producer atomic because using a mutex to protect it makes code hard to write. ### Verifying this change Create a separate namespace `public/test-backlog-quotas` to test the case when the backlog quota exceeds. Then add `testBacklogQuotasExceeded` test to make some backlog via creating a consumer and sending some messages to a partition of the topic. In this test, only 1 partition has backlog and will fail with the related error. So the test verifies that `createProducer` could return a correct error instead of `ResultUnknownError`.
codelipenghui
pushed a commit
that referenced
this pull request
May 20, 2022
…15161) Fixes #15078 When a partitioned producer is created and some of the partitioned failed to create, `closeAsync` will be called immediately, even if other partitions were still in progress of creating the associated single producers. Since `closeAsync` is called before calling `setFailed` on the `partitionedProducerCreatedPromise_` field, there is a race condition that all single producers are closed before the promise is set. Then the promise will be set with `ResultUnknownError`, see https://github.com/apache/pulsar/blob/4aeeed5dab9dfe9493526f36d539b3ef29cf6fe5/pulsar-client-cpp/lib/PartitionedProducerImpl.cc#L317. Only after all single producers failed or succeeded then call `closeAsync` if one of them failed. And ensure `partitionedProducerCreatedPromise_` was completed before calling `closeAsync`. This PR also makes the state of a partitioned producer atomic because using a mutex to protect it makes code hard to write. Create a separate namespace `public/test-backlog-quotas` to test the case when the backlog quota exceeds. Then add `testBacklogQuotasExceeded` test to make some backlog via creating a consumer and sending some messages to a partition of the topic. In this test, only 1 partition has backlog and will fail with the related error. So the test verifies that `createProducer` could return a correct error instead of `ResultUnknownError`. (cherry picked from commit 0f85596)
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
May 23, 2022
…pache#15161) Fixes apache#15078 When a partitioned producer is created and some of the partitioned failed to create, `closeAsync` will be called immediately, even if other partitions were still in progress of creating the associated single producers. Since `closeAsync` is called before calling `setFailed` on the `partitionedProducerCreatedPromise_` field, there is a race condition that all single producers are closed before the promise is set. Then the promise will be set with `ResultUnknownError`, see https://github.com/apache/pulsar/blob/4aeeed5dab9dfe9493526f36d539b3ef29cf6fe5/pulsar-client-cpp/lib/PartitionedProducerImpl.cc#L317. Only after all single producers failed or succeeded then call `closeAsync` if one of them failed. And ensure `partitionedProducerCreatedPromise_` was completed before calling `closeAsync`. This PR also makes the state of a partitioned producer atomic because using a mutex to protect it makes code hard to write. Create a separate namespace `public/test-backlog-quotas` to test the case when the backlog quota exceeds. Then add `testBacklogQuotasExceeded` test to make some backlog via creating a consumer and sending some messages to a partition of the topic. In this test, only 1 partition has backlog and will fail with the related error. So the test verifies that `createProducer` could return a correct error instead of `ResultUnknownError`. (cherry picked from commit 0f85596) (cherry picked from commit 95b5ef8)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #15078
Motivation
When a partitioned producer is created and some of the partitioned
failed to create,
closeAsyncwill be called immediately, even if otherpartitions were still in progress of creating the associated single
producers.
Since
closeAsyncis called before callingsetFailedon thepartitionedProducerCreatedPromise_field, there is a race conditionthat all single producers are closed before the promise is set. Then the
promise will be set with
ResultUnknownError, seepulsar/pulsar-client-cpp/lib/PartitionedProducerImpl.cc
Line 317 in 4aeeed5
Modifications
Only after all single producers failed or succeeded then call
closeAsyncif one of them failed. And ensurepartitionedProducerCreatedPromise_was completed before callingcloseAsync.This PR also makes the state of a partitioned producer atomic because
using a mutex to protect it makes code hard to write.
Verifying this change
Create a separate namespace
public/test-backlog-quotasto test thecase when the backlog quota exceeds. Then add
testBacklogQuotasExceededtest to make some backlog via creating aconsumer and sending some messages to a partition of the topic.
In this test, only 1 partition has backlog and will fail with the
related error. So the test verifies that
createProducercould return acorrect error instead of
ResultUnknownError.Documentation
Check the box below or label this PR directly.
Need to update docs?
doc-required(Your PR needs to update docs and you will update later)
no-need-doc(Please explain why)
doc(Your PR contains doc changes)
doc-added(Docs have been already added)