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 2023/01/09 14:01:51 UTC

[GitHub] [pulsar-client-cpp] shibd opened a new pull request, #170: [feature] Support set ServiceUrlProvider when create client.

shibd opened a new pull request, #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170

   Master Issue: https://github.com/apache/pulsar/pull/2543
   
   ### Motivation
   
   #163 
   
   `ServiceUrlProvider` is provided so that users can dynamically update serviceUrl, and can be easily implemented [#164 cluster level auto failover](https://github.com/apache/pulsar-client-cpp/issues/164).
   
   ### Modifications
   - Add the `ServiceUrlProvider` interface.
   - Add the `updateServiceUrl` method in the Client.
   - `Client` add new constructor method to support set  `ServiceUrlProvider` instance param.
   
   ### Verifying this change
   - Add `ServiceUrlProviderTest` to cover custom `ServiceUrlProvider`.
   
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [x] `doc-required` 
   (Your PR needs to update docs and you will update later)
   
   - [ ] `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-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080753416


##########
lib/ClientImpl.cc:
##########
@@ -497,6 +497,20 @@ void ClientImpl::getPartitionsForTopicAsync(const std::string& topic, GetPartiti
                   std::placeholders::_2, topicName, callback));
 }
 
+Result ClientImpl::updateServiceUrl(const std::string& serviceUrl) {
+    LOG_INFO("Updating service URL to " << serviceUrl);
+
+    try {
+        serviceNameResolver_.updateServiceUrl(serviceUrl);
+        lookupServicePtr_->updateServiceUrl(serviceUrl);

Review Comment:
   Actually we don't need to add `updateServiceUrl` method to `LookupService` because in C++ client, all `LookupService` implementations just call  `serviceNameResolver_.updateServiceUrl(serviceUrl);`.
   
   Unlike Java client, `HttpLookupService` uses a `HttpClient` class that accepts a service URL.



-- 
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-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080826113


##########
include/pulsar/Client.h:
##########
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
      */
     Client(const std::string& serviceUrl, const ClientConfiguration& clientConfiguration);
 
+    /**
+     * Create a Pulsar client object connecting to the specified cluster address and using the default
+     * configuration.
+     *
+     * <p>Instead of specifying a static service URL string (with {@link #serviceUrl(String)}), an application
+     * can pass a {@link ServiceUrlProvider} function that dynamically provide a service URL.
+     *
+     * @param serviceUrlProvider The serviceUrlProvider used to generate ServiceUrl.
+     * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid url.
+     */
+    Client(ServiceUrlProvider serviceUrlProvider);

Review Comment:
   LGTM. Before this PR, `serviceUrl` is a required argument. But after this PR, only one of `serviceUrl` and `serviceUrlProvider` should be provided.



-- 
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-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1407908931

   @shibd We should delay this feature until a result of this [discussion](https://lists.apache.org/thread/cojz6qxpc7v0j8vy0w476ttt0f89zr3z).


-- 
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-client-cpp] shibd commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396552263

   > If we want to implement AutoClusterFailover and ControlledClusterFailover in future, we actually don't need to implement a function that returns the service URL. Instead, we only need to implement the following methods:
   
   Yes, Actually, the `ServiceUrlProvider ` implemented here is just convenient for the user to return a function in order to get the service URL from elsewhere.
   
   If we do not expose the set  `ServiceUrlProvider`, It's no problem too. 
   
   Use `ServiceUrlProvider`
   ```c++
   Client client([]() {
       // get service URL from store.
       auto serviceUrl = xxx
       return serviceUrl;
   })
   ```
   
   `ServiceUrlProvider` is not used
   ```c++
       // get service URL from store.
       auto serviceUrl = xxx
       Client client(serviceUrl);
   ```
   
   I exposed it just for ease of use.
   
   
   


-- 
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-client-cpp] shibd commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396581433

   > In short, a natural design is to save the `ServiceUrlProvider` in `Client` and call `getServiceUrl()` method (in C++ it's just a function invocation) each time we need a service URL. Maybe we need to maintain a key-value (`serviceUrl -> ServiceNameResolver`), when the `serviceUrlProvider()` returns a different result from the key, creating a new `ServiceNameResolver` for related lookup requests.
   
   I think you only need the client to provide the `updateServiceUrl` method. 
   
   Users can take advantage of this approach to implement their own services. (Pseudocode)
   
   
   ```c++
   class Store;
   class AutomaticClusterService {
      public:
       explicit AutomaticClusterService(const Store& store, const Client pulsarClient)
           : store_(store), pulsarClient_(pulsarClient) {
           serviceUrlCache_ = store.queryServiceUrl();
           updateServiceUrlInterval_ = boost::posix_time::seconds(60);
           runUpdateServiceurl();
       }
   
       void start() {
           runUpdateServiceurl();
       }
   
       const std::string& getServiceUrl(){
           return serviceUrlCache_;
       }
   
       const void runUpdateServiceurl() {
           timer_->expires_from_now(updateServiceUrlInterval_);
           auto availableServiceUrl = store_.queryServiceUrl();
           timer_->async_wait([this](const boost::system::error_code& ec) {
             // Maybe some other implementation
             if (serviceUrlCache_ != availableServiceUrl) {
                 serviceUrlCache_ = availableServiceUrl;
                 pulsarClient_.updateServiceUrl(serviceUrlCache_);
               }
               runUpdateServiceurl();
           });
       }
   
   
      private:
       // External storage services
       Store store_;
       const Client pulsarClient_;
       std::string serviceUrlCache_;
       std::shared_ptr<boost::asio::deadline_timer> timer_;
       boost::posix_time::time_duration updateServiceUrlInterval_;
   };
   
   Store store;
   Client client(store.queryServiceUrl());
   
   AutomaticClusterService  automaticClusterService(store, client);
   automaticClusterService.start();
   
   //....
   
   ```


-- 
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-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396500856

   > The initialize and close methods are redundant.
   
   I reviewed the `ServiceUrlProvider` in Java client again. **I have thought `getServiceUrl()` is called each time the service URL is needed (e.g. topic lookup) but it's actually not.** The update of the service URL is actually done by `PulsarClient#updateServiceUrl`, which is triggered by the periodical task of `ServiceUrlProvide#initialize`.
   
   If we want to implement `AutoClusterFailover` and `ControlledClusterFailover` in future, we actually don't need to implement a function that returns the service URL. Instead, we only need to implement the following methods:
   - PulsarClient#updateServiceUrl
   - PulsarClientImpl#updateAuthentication
   - PulsarClientImpl#updateTlsTrustCertsFilePath
   
   The way of using `ServiceUrlProvider` is bad. It's more like a provider that can modify the internal state of `PulsarClient`.
   
   For C++ client, this PR implements the similar way with Java that the `ServiceUrlProvider` (a function that returns the service URL) is only triggerred when constructing the `Client` instance. There will be no difference between
   
   ```c++
   ServiceUrlProvider provider = /* ... */;
   Client client(provider());
   ```
   
   and
   
   ```c++
   ServiceUrlProvider provider = /* ... */;
   String serviceUrl = provider();
   Client client(serviceUrl);
   ```
   
   l'm going to send an email to dev mail list to discuss.


-- 
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-client-cpp] shibd commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080757958


##########
include/pulsar/Client.h:
##########
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
      */
     Client(const std::string& serviceUrl, const ClientConfiguration& clientConfiguration);
 
+    /**
+     * Create a Pulsar client object connecting to the specified cluster address and using the default
+     * configuration.
+     *
+     * <p>Instead of specifying a static service URL string (with {@link #serviceUrl(String)}), an application
+     * can pass a {@link ServiceUrlProvider} function that dynamically provide a service URL.
+     *
+     * @param serviceUrlProvider The serviceUrlProvider used to generate ServiceUrl.
+     * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid url.
+     */
+    Client(ServiceUrlProvider serviceUrlProvider);

Review Comment:
   This request the user to choose between `Client(ServiceUrlProvider serviceUrlProvider);` and `Client(const std::string& serviceUrl);` constructors.
   
   If add `setServiceUrlProvider` to `ClientConfiguration`. When the user sets it this way:
   
   ```c++
   ClientConfiguration config;
   config.setServiceUrlProvider(serviceUrlProvider);
   Client client(serviceUrl);
   ```
   
   This will be confuse.
   



-- 
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-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396556883

   I have sent an email: https://lists.apache.org/thread/cojz6qxpc7v0j8vy0w476ttt0f89zr3z


-- 
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-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080755872


##########
lib/ServiceNameResolver.h:
##########
@@ -29,29 +29,33 @@ namespace pulsar {
 class ServiceNameResolver {
    public:
     ServiceNameResolver(const std::string& uriString)
-        : serviceUri_(uriString), numAddresses_(serviceUri_.getServiceHosts().size()) {
+        : serviceUri_(std::make_shared<ServiceURI>(uriString)),
+          numAddresses_(serviceUri_->getServiceHosts().size()) {
         assert(numAddresses_ > 0);  // the validation has been done in ServiceURI
     }
 
     ServiceNameResolver(const ServiceNameResolver&) = delete;
     ServiceNameResolver& operator=(const ServiceNameResolver&) = delete;
 
     bool useTls() const noexcept {
-        return serviceUri_.getScheme() == PulsarScheme::PULSAR_SSL ||
-               serviceUri_.getScheme() == PulsarScheme::HTTPS;
+        return serviceUri_->getScheme() == PulsarScheme::PULSAR_SSL ||
+               serviceUri_->getScheme() == PulsarScheme::HTTPS;
     }
 
     bool useHttp() const noexcept {
-        return serviceUri_.getScheme() == PulsarScheme::HTTP ||
-               serviceUri_.getScheme() == PulsarScheme::HTTPS;
+        return serviceUri_->getScheme() == PulsarScheme::HTTP ||
+               serviceUri_->getScheme() == PulsarScheme::HTTPS;
     }
 
     const std::string& resolveHost() {
-        return serviceUri_.getServiceHosts()[(numAddresses_ == 1) ? 0 : (index_++ % numAddresses_)];
+        return serviceUri_->getServiceHosts()[(numAddresses_ == 1) ? 0 : (index_++ % numAddresses_)];
     }
 
+    void updateServiceUrl(const std::string& urlString) { serviceUri_.reset(new ServiceURI(urlString)); }

Review Comment:
   There is a problem that if the `urlString` contains multiple service URLs, we have to update `numAddresses_` as well. A possible solution is moving `numAddresses_` into `ServiceURI`.



-- 
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-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396560323

   In short, a natural design is to save the `ServiceUrlProvider` in `Client` and call `getServiceUrl()` method (in C++ it's just a function invocation) each time we need a service URL. Maybe we need to maintain a key-value (`serviceUrl -> ServiceNameResolver`), when the `serviceUrlProvider()` returns a different result from the key, creating a new `ServiceNameResolver` for related lookup requests.


-- 
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-client-cpp] shibd commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396555980

   If you feel redundant to expose this method, I can remove it. What do you think? /cc @BewareMyPower 


-- 
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-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1432617693

   Close this PR for now.


-- 
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-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080746462


##########
include/pulsar/Client.h:
##########
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
      */
     Client(const std::string& serviceUrl, const ClientConfiguration& clientConfiguration);
 
+    /**
+     * Create a Pulsar client object connecting to the specified cluster address and using the default
+     * configuration.
+     *
+     * <p>Instead of specifying a static service URL string (with {@link #serviceUrl(String)}), an application
+     * can pass a {@link ServiceUrlProvider} function that dynamically provide a service URL.
+     *
+     * @param serviceUrlProvider The serviceUrlProvider used to generate ServiceUrl.
+     * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid url.
+     */
+    Client(ServiceUrlProvider serviceUrlProvider);

Review Comment:
   Instead of adding new constructor overloads, we should add `setServiceUrlProvider` to `ClientConfiguration`. 



-- 
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-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080751592


##########
lib/ClientImpl.cc:
##########
@@ -497,6 +497,20 @@ void ClientImpl::getPartitionsForTopicAsync(const std::string& topic, GetPartiti
                   std::placeholders::_2, topicName, callback));
 }
 
+Result ClientImpl::updateServiceUrl(const std::string& serviceUrl) {
+    LOG_INFO("Updating service URL to " << serviceUrl);
+
+    try {
+        serviceNameResolver_.updateServiceUrl(serviceUrl);
+        lookupServicePtr_->updateServiceUrl(serviceUrl);
+    } catch (const std::exception& e) {

Review Comment:
   Catch `std::invalid_argument` instead of `std::exception`



-- 
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-client-cpp] BewareMyPower commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1396625182

   > I think only need the client to provide the updateServiceUrl method.
   
   This idea also looks good to me.


-- 
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-client-cpp] shibd commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1381406248

   > We only need to support passing a std::function<std::string()>. The initialize and close methods are redundant.
   
    The initialize and close methods are useful. We need pass `Client&` to `ServiceUrlProvider` by `initialize` method.
   
   https://github.com/apache/pulsar-client-cpp/blob/7d91638513405ed3e9bc5e7cd782d30e8703679d/lib/Client.cc#L43
   
   This way, if the user implements the `ServiceUrlProvider`. When it listens for a change in the service URL (such as through zk or other storage), it can invoke the `client.updateServiceUrl` method to update the URL.
   
   


-- 
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-client-cpp] shibd commented on pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
shibd commented on PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#issuecomment-1381605478

   > We only need to support passing a std::function<std::string()>. The initialize and close methods are redundant.
   
   Yes, you are right. The initialize and close methods are redundant.
   
   In fact, The client already provides the `updateServiceUrl` method, which the user can use to implement dynamic update service URL. As for how the user implements it and how to close his resources, this is left to the user.
   
   I changed to pass a `std::function<const std::string&()>` when creating a `Client`.
   


-- 
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-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080749572


##########
lib/ServiceNameResolver.h:
##########
@@ -29,29 +29,33 @@ namespace pulsar {
 class ServiceNameResolver {

Review Comment:
   Add a `#include <memory>` to this file for `std::make_shared`. The compilation did not fail because the `.cc` files that include this header already include `<memory>`.



-- 
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-client-cpp] shibd commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
shibd commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080780926


##########
lib/ClientImpl.cc:
##########
@@ -497,6 +497,20 @@ void ClientImpl::getPartitionsForTopicAsync(const std::string& topic, GetPartiti
                   std::placeholders::_2, topicName, callback));
 }
 
+Result ClientImpl::updateServiceUrl(const std::string& serviceUrl) {
+    LOG_INFO("Updating service URL to " << serviceUrl);
+
+    try {
+        serviceNameResolver_.updateServiceUrl(serviceUrl);
+        lookupServicePtr_->updateServiceUrl(serviceUrl);

Review Comment:
   Ok, I will remove `updateServiceUrl`  method of `LookupService `.



-- 
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-client-cpp] BewareMyPower commented on a diff in pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #170:
URL: https://github.com/apache/pulsar-client-cpp/pull/170#discussion_r1080827497


##########
include/pulsar/Client.h:
##########
@@ -66,6 +67,31 @@ class PULSAR_PUBLIC Client {
      */
     Client(const std::string& serviceUrl, const ClientConfiguration& clientConfiguration);
 
+    /**
+     * Create a Pulsar client object connecting to the specified cluster address and using the default
+     * configuration.
+     *
+     * <p>Instead of specifying a static service URL string (with {@link #serviceUrl(String)}), an application
+     * can pass a {@link ServiceUrlProvider} function that dynamically provide a service URL.
+     *
+     * @param serviceUrlProvider The serviceUrlProvider used to generate ServiceUrl.
+     * @throw std::invalid_argument if `serviceUrlProvider()` return a invalid url.
+     */
+    Client(ServiceUrlProvider serviceUrlProvider);

Review Comment:
   ```suggestion
       /**
        * @see Client(ServiceUrlProvider, const ClientConfiguration&)
        */
       Client(ServiceUrlProvider serviceUrlProvider) : Client(serviceUrlProvider, ClientConfiguration{}) {}
   ```
   
   Avoid unnecessary duplicated code and 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-client-cpp] BewareMyPower closed pull request #170: [feature] Support set ServiceUrlProvider when create client.

Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower closed pull request #170: [feature] Support set ServiceUrlProvider when create client.
URL: https://github.com/apache/pulsar-client-cpp/pull/170


-- 
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