Ok, the last two methods IMO are a completely different topic.
I wonder what the maintainers think about this.
On Mon, Jan 30, 2023 at 4:16 AM Yunze Xu <y...@streamnative.io.invalid>
wrote:
> In summary, the core issue is that ServiceUrlProvider could be
> misused. A user might want to implement its own ServiceUrlProvider by
> changing the returned value of `get` method but it actually does not
> work. The built-in implementations like AutoFailover actually work by
> these `updateXXX` methods. So it's meaningless to provide them as
> ServiceUrlProvider implementations.
>
> To answer your question directly: totally yes. But not only
> `setServiceUrl` (the name is `updateServiceUrl` in code), the
> `updateAuthentication` and `updateTlsTrustCertsFilePath` methods,
> which are not exposed to `PulsarClient`, should also be exposed.
>
> Thanks,
> Yunze
>
> On Sun, Jan 29, 2023 at 11:57 PM Asaf Mesika <asaf.mes...@gmail.com>
> wrote:
> >
> > Yunze - in summary - your proposal is to get rid of the
> ServiceUrlProvider
> > right? You just want to have setServiceUrl on the client instead?
> >
> >
> > On Wed, Jan 25, 2023 at 2:03 PM Yunze Xu <y...@streamnative.io.invalid>
> > wrote:
> >
> > > It makes sense to me. So the better way is still providing the
> > > `updateXXX` methods to `PulsarClient`. As for how to detect the
> > > connection info (e.g. service URL) changes, it's determined by the
> > > user's implementation. For example, the current AutoClusterFailover
> > > polls with a fixed interval.
> > >
> > > We can also implement a notification mechanism based on the
> > > `updateXXX` methods. For example, assuming we have a producer that
> > > writes the latest service URL to a topic and a consumer that reads the
> > > latest service URL from that topic. Then, the consumer could call
> > > `pulsarClient.updateXXX` once it receives the latest service URL.
> > >
> > > Thanks,
> > > Yunze
> > >
> > > On Tue, Jan 24, 2023 at 4:32 PM Asaf Mesika <asaf.mes...@gmail.com>
> wrote:
> > > >
> > > > 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
> > > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > >
> > >
>
