You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "merlimat (via GitHub)" <gi...@apache.org> on 2023/10/30 23:19:36 UTC
[PR] Added support for multiple connections to each broker [pulsar-client-cpp]
merlimat opened a new pull request, #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336
<!-- Either this PR fixes an issue, -->
Fixes #221
### Motivation
Support having more than 1 connection to each broker.
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376984314
##########
lib/ConnectionPool.cc:
##########
@@ -34,13 +34,13 @@ DECLARE_LOG_OBJECT()
namespace pulsar {
ConnectionPool::ConnectionPool(const ClientConfiguration& conf, ExecutorServiceProviderPtr executorProvider,
- const AuthenticationPtr& authentication, bool poolConnections,
- const std::string& clientVersion)
+ const AuthenticationPtr& authentication, const std::string& clientVersion)
: clientConfiguration_(conf),
executorProvider_(executorProvider),
authentication_(authentication),
- poolConnections_(poolConnections),
- clientVersion_(clientVersion) {}
+ clientVersion_(clientVersion),
+ randomDistribution_(0, conf.getConnectionsPerBroker()),
Review Comment:
From the [doc](https://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution), the range is left-closed and right-closed: [a,b].
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376964472
##########
lib/ClientConfiguration.cc:
##########
@@ -40,6 +40,13 @@ ClientConfiguration& ClientConfiguration::setMemoryLimit(uint64_t memoryLimitByt
uint64_t ClientConfiguration::getMemoryLimit() const { return impl_->memoryLimit; }
+ClientConfiguration& ClientConfiguration::setConnectionsPerBroker(int connectionsPerBroker) {
+ impl_->connectionsPerBroker = connectionsPerBroker;
Review Comment:
Should we add an invalidation here?
```c++
if (connectionsPerBroker <= 0) {
throw std::invalid_argument("connectionsPerBroker should be greater than 0");
}
```
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1378220666
##########
lib/ConnectionPool.cc:
##########
@@ -107,7 +108,7 @@ Future<Result, ClientConnectionWeakPtr> ConnectionPool::getConnectionAsync(
LOG_INFO("Created connection for " << logicalAddress);
Review Comment:
It could be confusing though to see a `broker-5-N` in the log and will be mistaken by a different host name
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376988417
##########
lib/ConnectionPool.cc:
##########
@@ -34,13 +34,13 @@ DECLARE_LOG_OBJECT()
namespace pulsar {
ConnectionPool::ConnectionPool(const ClientConfiguration& conf, ExecutorServiceProviderPtr executorProvider,
- const AuthenticationPtr& authentication, bool poolConnections,
- const std::string& clientVersion)
+ const AuthenticationPtr& authentication, const std::string& clientVersion)
: clientConfiguration_(conf),
executorProvider_(executorProvider),
authentication_(authentication),
- poolConnections_(poolConnections),
- clientVersion_(clientVersion) {}
+ clientVersion_(clientVersion),
+ randomDistribution_(0, conf.getConnectionsPerBroker()),
Review Comment:
```suggestion
randomDistribution_(0, conf.getConnectionsPerBroker() - 1),
```
The range is left-closed and right-closed. We need to `-1` here to make sure the total number of the connections to be equal to `conf.getConnectionsPerBroker()`.
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376990807
##########
lib/ConnectionPool.cc:
##########
@@ -34,13 +34,13 @@ DECLARE_LOG_OBJECT()
namespace pulsar {
ConnectionPool::ConnectionPool(const ClientConfiguration& conf, ExecutorServiceProviderPtr executorProvider,
- const AuthenticationPtr& authentication, bool poolConnections,
- const std::string& clientVersion)
+ const AuthenticationPtr& authentication, const std::string& clientVersion)
: clientConfiguration_(conf),
executorProvider_(executorProvider),
authentication_(authentication),
- poolConnections_(poolConnections),
- clientVersion_(clientVersion) {}
+ clientVersion_(clientVersion),
+ randomDistribution_(0, conf.getConnectionsPerBroker()),
Review Comment:
Yeah this comment is right.
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376980677
##########
lib/ClientConfiguration.cc:
##########
@@ -40,6 +40,13 @@ ClientConfiguration& ClientConfiguration::setMemoryLimit(uint64_t memoryLimitByt
uint64_t ClientConfiguration::getMemoryLimit() const { return impl_->memoryLimit; }
+ClientConfiguration& ClientConfiguration::setConnectionsPerBroker(int connectionsPerBroker) {
+ impl_->connectionsPerBroker = connectionsPerBroker;
Review Comment:
+1
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376992566
##########
lib/ConnectionPool.cc:
##########
@@ -107,7 +108,7 @@ Future<Result, ClientConnectionWeakPtr> ConnectionPool::getConnectionAsync(
LOG_INFO("Created connection for " << logicalAddress);
Review Comment:
Maybe we can print `key` instead of `logicalAddress` now. Otherwise users might not know which specific connection is created for `logicalAddress`.
```c++
LOG_INFO("Created connection for " << key);
```
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376988417
##########
lib/ConnectionPool.cc:
##########
@@ -34,13 +34,13 @@ DECLARE_LOG_OBJECT()
namespace pulsar {
ConnectionPool::ConnectionPool(const ClientConfiguration& conf, ExecutorServiceProviderPtr executorProvider,
- const AuthenticationPtr& authentication, bool poolConnections,
- const std::string& clientVersion)
+ const AuthenticationPtr& authentication, const std::string& clientVersion)
: clientConfiguration_(conf),
executorProvider_(executorProvider),
authentication_(authentication),
- poolConnections_(poolConnections),
- clientVersion_(clientVersion) {}
+ clientVersion_(clientVersion),
+ randomDistribution_(0, conf.getConnectionsPerBroker()),
Review Comment:
```suggestion
randomDistribution_(0, conf.getConnectionsPerBroker() -1),
```
The range is left-closed and right-closed. We need to `-1` here to make sure the total number of the connections to be equal to `conf.getConnectionsPerBroker()`.
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376963053
##########
lib/ConnectionPool.cc:
##########
@@ -34,13 +34,13 @@ DECLARE_LOG_OBJECT()
namespace pulsar {
ConnectionPool::ConnectionPool(const ClientConfiguration& conf, ExecutorServiceProviderPtr executorProvider,
- const AuthenticationPtr& authentication, bool poolConnections,
- const std::string& clientVersion)
+ const AuthenticationPtr& authentication, const std::string& clientVersion)
: clientConfiguration_(conf),
executorProvider_(executorProvider),
authentication_(authentication),
- poolConnections_(poolConnections),
- clientVersion_(clientVersion) {}
+ clientVersion_(clientVersion),
+ randomDistribution_(0, conf.getConnectionsPerBroker()),
Review Comment:
```suggestion
randomDistribution_(0, conf.getConnectionsPerBroker() - 1),
```
~~The `uniform_int_distribution` range is left-closed and right-open~~
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1378312629
##########
lib/ConnectionPool.cc:
##########
@@ -107,7 +108,7 @@ Future<Result, ClientConnectionWeakPtr> ConnectionPool::getConnectionAsync(
LOG_INFO("Created connection for " << logicalAddress);
Review Comment:
Okay. It makes sense.
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376990180
##########
lib/ConnectionPool.cc:
##########
@@ -34,13 +34,13 @@ DECLARE_LOG_OBJECT()
namespace pulsar {
ConnectionPool::ConnectionPool(const ClientConfiguration& conf, ExecutorServiceProviderPtr executorProvider,
- const AuthenticationPtr& authentication, bool poolConnections,
- const std::string& clientVersion)
+ const AuthenticationPtr& authentication, const std::string& clientVersion)
: clientConfiguration_(conf),
executorProvider_(executorProvider),
authentication_(authentication),
- poolConnections_(poolConnections),
- clientVersion_(clientVersion) {}
+ clientVersion_(clientVersion),
+ randomDistribution_(0, conf.getConnectionsPerBroker()),
Review Comment:
My fault. It's true that `uniform_int_distribution` uses right-closed range.
I wanted to say we should use a left-closed and right-open range: `[0, connectionsPerBroker)` so the initialization of `uniform_int_distribution` should be:
```c++
randomDistribution_(0, conf.getConnectionsPerBroker() - 1),
```
@RobertIndie
--
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
Re: [PR] Added support for multiple connections to each broker [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #336:
URL: https://github.com/apache/pulsar-client-cpp/pull/336#discussion_r1376963053
##########
lib/ConnectionPool.cc:
##########
@@ -34,13 +34,13 @@ DECLARE_LOG_OBJECT()
namespace pulsar {
ConnectionPool::ConnectionPool(const ClientConfiguration& conf, ExecutorServiceProviderPtr executorProvider,
- const AuthenticationPtr& authentication, bool poolConnections,
- const std::string& clientVersion)
+ const AuthenticationPtr& authentication, const std::string& clientVersion)
: clientConfiguration_(conf),
executorProvider_(executorProvider),
authentication_(authentication),
- poolConnections_(poolConnections),
- clientVersion_(clientVersion) {}
+ clientVersion_(clientVersion),
+ randomDistribution_(0, conf.getConnectionsPerBroker()),
Review Comment:
```suggestion
randomDistribution_(0, conf.getConnectionsPerBroker() + 1),
```
The `uniform_int_distribution` range is left-closed and right-open
--
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