If I understand correctly, the idea of having the ability to change
serviceUrl is to support switching over between clusters dynamically?
If that is the case, doesn't it make sense that it will trigger the change
to the client instead of the client polling it and check it self if
something has changed?
On Mon, Jan 23, 2023 at 5:00 AM Yunze Xu <y...@streamnative.io.invalid>
wrote:
> Yes. The poll happens in the client's internal thread.
>
> Thanks,
> Yunze
>
> On Sun, Jan 22, 2023 at 6:56 PM Asaf Mesika <asaf.mes...@gmail.com> wrote:
> >
> > and the client will poll the ConnectInfoProvider and check if something
> was
> > changed?
> >
> > On Fri, Jan 20, 2023 at 11:19 AM Yunze Xu <y...@streamnative.io.invalid>
> > wrote:
> >
> > > > 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
> > > > > >
> > > > >
> > >
>
