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 2022/09/02 16:03:59 UTC
[GitHub] [pulsar] coderzc opened a new pull request, #17439: [improve][client-c++] support Exclusive Producer access mode for c++
coderzc opened a new pull request, #17439:
URL: https://github.com/apache/pulsar/pull/17439
Master Issue: #17169
### Modifications
Support Exclusive Producer access mode for c++ by adding `accessMode`config in the ProducerConfiguration.
`ExclusiveWithFencing` and `ExclusiveWithFencing` will be supported by other PR.
### 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)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #17439:
URL: https://github.com/apache/pulsar/pull/17439
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r971727441
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -204,7 +206,8 @@ void ProducerImpl::handleCreateProducer(const ClientConnectionPtr& cnx, Result r
}
// if the producer is lazy the send timeout timer is already running
- if (!conf_.getLazyStartPartitionedProducers()) {
+ if (!(conf_.getLazyStartPartitionedProducers() &&
+ conf_.getAccessMode() == ProducerConfiguration::Shared)) {
Review Comment:
So in C++ client, it checks `conf_.getLazyStartPartitionedProducers()` to verify if the owner of `ProducerImpl` is a `PartitionedProducerImpl`. This check is wrong. Because for a producer on a non-partitioned topic, `conf_.getLazyStartPartitionedProducers()` should not affect the existing logic.
There should be a method to check if a `ProducerImpl` is created from a `PartitionedProducerImpl`. I think we can check the `partition_` field.
Anyway, it doesn't affect this PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r966524980
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -204,7 +206,8 @@ void ProducerImpl::handleCreateProducer(const ClientConnectionPtr& cnx, Result r
}
// if the producer is lazy the send timeout timer is already running
- if (!conf_.getLazyStartPartitionedProducers()) {
+ if (!(conf_.getLazyStartPartitionedProducers() &&
+ conf_.getAccessMode() == ProducerConfiguration::Shared)) {
Review Comment:
Because the previous operation check the access mode.
```
if (conf_.getLazyStartPartitionedProducers() && conf_.getAccessMode() == ProducerConfiguration::Shared) {
// we need to kick it off now as it is possible that the connection may take
// longer than sendTimeout to connect
startSendTimeoutTimer();
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r971723869
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -204,7 +206,8 @@ void ProducerImpl::handleCreateProducer(const ClientConnectionPtr& cnx, Result r
}
// if the producer is lazy the send timeout timer is already running
- if (!conf_.getLazyStartPartitionedProducers()) {
+ if (!(conf_.getLazyStartPartitionedProducers() &&
+ conf_.getAccessMode() == ProducerConfiguration::Shared)) {
Review Comment:
I got it. I think it's because the previous implementation of lazy producer is somehow incorrect.
In Java client, when a `ProducerImpl` is created, it starts a send timer and call `grabCnx()`. So for a `PartitionedProducerImpl` object, Java client doesn't create a `ProducerImpl` if lazy start is enabled and the access mode is `Shared`.
However, in C++ client, the send timer is started after `grabCnx()` is called, which are both called in `ProducerImpl::start`, while the creation of a `ProducerImpl` doesn't do anything.
Regarding this PR, I think you can just keep the existing logic.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r966175773
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -204,7 +206,8 @@ void ProducerImpl::handleCreateProducer(const ClientConnectionPtr& cnx, Result r
}
// if the producer is lazy the send timeout timer is already running
- if (!conf_.getLazyStartPartitionedProducers()) {
+ if (!(conf_.getLazyStartPartitionedProducers() &&
+ conf_.getAccessMode() == ProducerConfiguration::Shared)) {
Review Comment:
Why did you check the access mode here?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r971723869
##########
pulsar-client-cpp/lib/ProducerImpl.cc:
##########
@@ -204,7 +206,8 @@ void ProducerImpl::handleCreateProducer(const ClientConnectionPtr& cnx, Result r
}
// if the producer is lazy the send timeout timer is already running
- if (!conf_.getLazyStartPartitionedProducers()) {
+ if (!(conf_.getLazyStartPartitionedProducers() &&
+ conf_.getAccessMode() == ProducerConfiguration::Shared)) {
Review Comment:
I got it. I think it's because the previous implementation of lazy producer is somehow incorrect.
In Java client, when a `ProducerImpl` is created, it starts a send timer and call `grabCnx()`. So for a `PartitionedProducerImpl` object, **Java client doesn't create a `ProducerImpl` if lazy start is enabled** and the access mode is `Shared`.
However, in C++ client, the send timer is started after `grabCnx()` is called, which are both called in `ProducerImpl::start`, while the creation of a `ProducerImpl` doesn't do anything. So **C++ client still creates a `ProducerImpl` if lazy start is enabled** for a partitioned topic, but it doesn't call `start()` method.
Regarding this PR, I think you can just keep the existing logic.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r964931914
##########
pulsar-client-cpp/include/pulsar/ProducerConfiguration.h:
##########
@@ -501,6 +506,12 @@ class PULSAR_PUBLIC ProducerConfiguration {
*/
bool isChunkingEnabled() const;
+ ProducerConfiguration& setAccessMode(const ProducerAccessMode& mode);
+
+ ProducerAccessMode getAccessMode() const;
+
+ bool isLazyStartPartitionedProducers() const;
Review Comment:
Unfortunately we already have `getLazyStartPartitionedProducers`, though the name might not be good. Please don't introduce a new getter.
##########
pulsar-client-cpp/include/pulsar/ProducerConfiguration.h:
##########
@@ -501,6 +506,12 @@ class PULSAR_PUBLIC ProducerConfiguration {
*/
bool isChunkingEnabled() const;
+ ProducerConfiguration& setAccessMode(const ProducerAccessMode& mode);
+
+ ProducerAccessMode getAccessMode() const;
Review Comment:
Please add the API docs
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#issuecomment-1236836205
> conf.getAccessMode() == ProducerAccessMode.Shared
Thanks, I check the java client code, and only `shared` access mode can be operation logic introduced by #10279, I compatible with it. Please review it again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
coderzc commented on PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#issuecomment-1236482859
@BewareMyPower @shibd @RobertIndie PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r962480223
##########
pulsar-client-cpp/include/pulsar/ProducerConfiguration.h:
##########
@@ -27,6 +27,7 @@
#include <pulsar/ProducerCryptoFailureAction.h>
#include <pulsar/CryptoKeyReader.h>
#include <pulsar/Schema.h>
+#include <lib/PulsarApi.pb.h>
Review Comment:
`PulsarApi.pb.h` should not be exposed to user.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r964978428
##########
pulsar-client-cpp/include/pulsar/ProducerConfiguration.h:
##########
@@ -501,6 +506,12 @@ class PULSAR_PUBLIC ProducerConfiguration {
*/
bool isChunkingEnabled() const;
+ ProducerConfiguration& setAccessMode(const ProducerAccessMode& mode);
+
+ ProducerAccessMode getAccessMode() const;
+
+ bool isLazyStartPartitionedProducers() const;
Review Comment:
Reuse `getLazyStartPartitionedProducers` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] coderzc commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r964978428
##########
pulsar-client-cpp/include/pulsar/ProducerConfiguration.h:
##########
@@ -501,6 +506,12 @@ class PULSAR_PUBLIC ProducerConfiguration {
*/
bool isChunkingEnabled() const;
+ ProducerConfiguration& setAccessMode(const ProducerAccessMode& mode);
+
+ ProducerAccessMode getAccessMode() const;
+
+ bool isLazyStartPartitionedProducers() const;
Review Comment:
Reuse `getLazyStartPartitionedProducers` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #17439: [improve][client-c++] support Exclusive Producer access mode for c++
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #17439:
URL: https://github.com/apache/pulsar/pull/17439#discussion_r965583109
##########
pulsar-client-cpp/lib/ProducerConfiguration.cc:
##########
@@ -211,7 +211,7 @@ ProducerConfiguration& ProducerConfiguration::setLazyStartPartitionedProducers(
}
bool ProducerConfiguration::getLazyStartPartitionedProducers() const {
- return impl_->useLazyStartPartitionedProducers;
+ return impl_->useLazyStartPartitionedProducers && impl_->accessMode == Shared;
Review Comment:
It's a trivial getter, we should not check other fields. Instead, we should call these two getters in `PartitionedProducerImpl` like https://github.com/apache/pulsar/blob/2848fa0da09e035951220c3d04138041e1477e60/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PartitionedProducerImpl.java#L91-L92
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org