> I think the `updateServiceUrl` is not the initial purpose of exposing to the
> Client API.
I agree. We might need an API like
```java
ClientBuilder serviceUrlProvider(ServiceUrlProvider
serviceUrlProvider, Duration interval)
```
But not only the service URL, as you can see, the
`AutoClusterFailover` implementation is already beyond the scope of
service URL, it uses two internal APIs `updateAuthentication` and
`updateTlsTrustCertsFilePath` to update other states of
PulsarClientImpl.
> So from the user's perspective, they only need to apply a service URL
> provided to the client
Yes. That's when I thought of when I saw the C++ client catch up [1].
The `initialize` and `close` methods are not necessary. If let me
design the interface, I would like the following solution, which is
more simple and can accept a lambda.
```java
public interface ServiceUrlProvider {
String getServiceUrl();
}
```
However, as I've mentioned again, now authentication and TLS info also
need updates, so I have an initial design like:
```java
public class ConnectInfo {
String serviceUrl;
Authentication authentication;
String tlsCertificatesFile;
// ...
}
interface ConnectInfoProvider extends Supplier<ConnectInfo> {
}
```
When configuring the `ConnectInfoProvider`, we should provide an interval.
And I'm going to open a PIP for it to deprecate `ServiceUrlProvider`.
BTW, I found this issue when I reviewed the PR to migrate the
ServiceUrlProvider into C++ client. IMO, the current design in Java
client is bad and I don't want to adopt the same design in C++ client.
Thanks,
Yunze
On Thu, Jan 19, 2023 at 4:51 PM PengHui Li <peng...@apache.org> wrote:
>
> Is it better to introduce a service URL detect interval to the service URL
> provider?
> I think the `updateServiceUrl` is not the initial purpose of exposing to
> the Client API.
>
> It looks like users just provide the interval of checking whether the
> service URL is changed.
> The Pulsar client will check it automatically. Using updateServiceUrl can
> also achieve the purpose,
> but users need to provide a fake service URL first or fetch the service URL
> before
> creating the client.
>
> Another reason we need service URL provider API is that one team will
> usually
> provide an extra pulsar client lib with the service URL provider
> implementation.
> The lib
> can be used across multiple teams. So from the user's perspective, they
> only need to apply
> a service URL provided to the client, they don't care about what is the
> service URL and how to
> update it.
>
> Thanks,
> Penghui
>
> On Thu, Jan 19, 2023 at 3:43 PM Baodi Shi <ba...@apache.org> wrote:
>
> > Hi, Yunze:
> >
> > Obviously, the `ServiceUrlProvider` config is redundant.
> > >
> >
> > Agree. In fact, The client already provides the updateServiceUrl method,
> > which the user can use to implement a dynamic update service URL. As for
> > how the user implements it and how to close his resources, I think it can
> > be left to the user.
> >
> >
> > 在 2023年1月19日 15:16:52 上，Yunze Xu <y...@streamnative.io.invalid> 写道：
> >
> > > Hi all,
> > >
> > > Currently we have a `ServiceUrlProvider` interface to configure when
> > > constructing a PulsarClient in `ClientBuilder#serviceUrlProvider`.
> > > From the beginning, I thought the `getServiceUrl` method is called
> > > each time the service URL is used, e.g. topic metadata lookup.
> > > However, the `getServiceUrl` method is only called when constructing
> > > the PulsarClient object. To update the PulsarClient's internal service
> > > URL, `PulsarClient#updateServiceUrl` must be called. Therefore, if we
> > > want to implement a `ServiceUrlProvider` that retrieves the latest
> > > service URL from a database, I have to implement it like:
> > >
> > > ```java
> > > class DataBaseServiceUrlProvider implements ServiceUrlProvider {
> > >
> > > private final ScheduledExecutorService executor =
> > > Executors.newSingleThreadScheduledExecutor();
> > >
> > > @Override
> > > public void initialize(PulsarClient client) {
> > > executor.schedule(() -> {
> > > try {
> > > client.updateServiceUrl(readServiceUrlFromDB()/* a
> > > fake method */);
> > > } catch (PulsarClientException e) {
> > > throw new RuntimeException(e);
> > > }
> > > }, 1, TimeUnit.SECONDS);
> > > }
> > >
> > > @Override
> > > public String getServiceUrl() {
> > > return "pulsar://localhost:6650";
> > > }
> > >
> > > @Override
> > > public void close() {
> > > executor.shutdown();
> > > }
> > > }
> > > ```
> > >
> > > The key point is, if we didn't call `client.updateServiceUrl` and only
> > > modified the returned value of `getServiceUrl` periodically, the
> > > internal service URL would never be updated.
> > >
> > > Based on the provider above, the following two code snippets could be
> > > nearly the same.
> > >
> > > ```java
> > > var client = PulsarClient.builder().serviceUrlProvider(new
> > > DataBaseServiceUrlProvider()).build();
> > > /* ... */
> > > client.close();
> > > ```
> > >
> > > ```java
> > > var client =
> > > PulsarClient.builder().serviceUrl("pulsar://localhost:6650").build();
> > > var provider = new DataBaseServiceUrlProvider();
> > > provider.initialize(client);
> > > /* ... */
> > > provider.close();
> > > client.close();
> > > ```
> > >
> > > Obviously, the `ServiceUrlProvider` config is redundant.
> > >
> > > PIP-121 implements the `AutoClusterFailover` as the service URL
> > > provider. However, it also calls the following methods periodically:
> > > - PulsarClientImpl#updateAuthentication
> > > - PulsarClientImpl#updateTlsTrustCertsFilePath
> > >
> > > It's unnatural and intuitive to say a service URL provider could
> > > modify the internal states of `PulsarClient`, including:
> > > - the service URL
> > > - the authentication
> > > - the TLS trust certificate file
> > > - ...
> > >
> > > BTW, the implementation of PIP-121 [1] is different from the design [2].
> > >
> > > [1] https://github.com/apache/pulsar/pull/13316
> > > [2] https://github.com/apache/pulsar/issues/13315
> > >
> > > Thanks,
> > > Yunze
> > >
> >
