Not completely different IMO. When we want to update the service URL,
we usually want to connect to another cluster. However, the cluster
could have configured authentication, TLS for security, but we cannot
guarantee two clusters share the same TLS certificates or
authentication configs. I think we should group these necessary
information into a common class like `ConnectInfo`. Then, just expose
a method to update the `ConnectInfo` so that code could be more
scalable.
Thanks,
Yunze
On Thu, Feb 2, 2023 at 9:39 PM Asaf Mesika <asaf.mes...@gmail.com> wrote:
>
> 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
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >
