Re: [DISCUSS] ClientVersion conflict across multiple language client

[Zike Yang <zi...@apache.org>]

Hi, Michael,

> I don't think we need to make it configurable because the library
owner, not the user, has full control when creating the Connect
command.

The Nodejs, Python, and C client are all based on the C++ client. Same
for the Scala client which is based on the Java client. They have no
control when creating the CommandConnect. But we need to make them
able to set the client version to resolve this conflict issue. I think
it is still worth making it configurable.

WDYT? Do you have any ideas?

Thanks,
Zike Yang

On Fri, Feb 24, 2023 at 3:24 PM Zike Yang <zi...@apache.org> wrote:
>
> > I think it makes sense to make the format
> "pulsar-<language>-<version>" for official clients. We could then
> recommend third party clients replace "pulsar" with a relevant org
> name. There isn't a way to enforce the version string though, so these
> would only be recommendations.
>
> +1 for this format.
>
> > F#:
> I'm not exactly sure how to interpret this input.
> https://github.com/fsprojects/pulsar-client-dotnet/blob/5c6dcff0184f87005f55dcc847893a889aaee2ad/src/Pulsar.Client/Internal/ClientCnx.fs#L119
>
> The format is like `2.11.0.0`. It's consistent with the current java
> client version format.
>
> > Because we filter out the official go client, I propose that we remove
> the filtering of certain client versions:
> https://github.com/apache/pulsar/pull/19616.
>
> Thanks for your PR. I have approved.
>
> Since there are no objections to this discussion, I will next make the
> client version format of all clients(at least official clients)
> consistent.
>
> BR,
> Zike Yang
>
> On Fri, Feb 24, 2023 at 3:22 AM Michael Marshall <mm...@apache.org> wrote:
> >
> > > Go:
> > > ClientVersionString = "Pulsar Go " + Version
> > > https://github.com/apache/pulsar-client-go/blob/dedbdc45c63b06e6a12356785418cd906d6bab3c/pulsar/internal/version.go#L43
> >
> > Because we filter out the official go client, I propose that we remove
> > the filtering of certain client versions:
> > https://github.com/apache/pulsar/pull/19616.
> >
> > Thanks,
> > Michael
> >
> > On Thu, Feb 23, 2023 at 12:45 PM Michael Marshall <mm...@apache.org> wrote:
> > >
> > > I found another data point in favor of moving in this direction. Our
> > > protocol spec says the following about the client_version field:
> > >
> > > message CommandConnect {
> > > "client_version" : "Pulsar-Client-Java-v1.15.2",
> > > "auth_method_name" : "my-authentication-plugin",
> > > "auth_data" : "my-auth-data",
> > > "protocol_version" : 6
> > > }
> > >
> > > * `client_version`: String-based identifier. Format is not enforced.
> > >
> > > https://github.com/apache/pulsar-site/blame/main/docs/developing-binary-protocol.md#L133
> > >
> > > I think it makes sense to make the format
> > > "pulsar-<language>-<version>" for official clients. We could then
> > > recommend third party clients replace "pulsar" with a relevant org
> > > name. There isn't a way to enforce the version string though, so these
> > > would only be recommendations.
> > >
> > > For official clients, here are our version strings:
> > >
> > > Go:
> > > ClientVersionString = "Pulsar Go " + Version
> > > https://github.com/apache/pulsar-client-go/blob/dedbdc45c63b06e6a12356785418cd906d6bab3c/pulsar/internal/version.go#L43
> > >
> > > C++
> > > connect->set_client_version(PULSAR_VERSION_STR);
> > > https://github.com/apache/pulsar-client-cpp/blob/96507cecdc90f10eca5febc48a0623f72d338615/lib/Commands.cc#L269
> > > std::string version = std::string("Pulsar-CPP-v") + PULSAR_VERSION_STR;
> > > https://github.com/apache/pulsar-client-cpp/blob/05807bdaf3a4341b22efd7c71f7b00c47cc31413/lib/HTTPLookupService.cc#L195
> > >
> > > I took a quick survey of some existing third party clients. Here are
> > > the results for how the version string is written:
> > >
> > > Rust:
> > > client_version: String::from("2.0.1-incubating"),
> > > https://github.com/streamnative/pulsar-rs/blob/eb87ef796bd8cb42fc72bb72ed14751fffa437e1/src/connection.rs#L1173
> > >
> > > Haskell:
> > > & F.clientVersion .~ "Pulsar-Client-Haskell-v" <> T.pack (showVersion version)
> > > https://github.com/cr-org/supernova/blob/602409a18f47a38541ba24f5e885199efd383f48/lib/src/Pulsar/Protocol/Commands.hs#L22
> > >
> > > PHP:
> > > $cmd->setClientVersion(sprintf('ikilobyte/pulsar-client-php@v%s',
> > > Client::VERSION_ID));
> > > https://github.com/ikilobyte/pulsar-client-php/blob/e8847c62d3bf136886312a14a04f51c59ca99886/src/IO/StreamIO.php#L77
> > >
> > > F#:
> > > I'm not exactly sure how to interpret this input.
> > > https://github.com/fsprojects/pulsar-client-dotnet/blob/5c6dcff0184f87005f55dcc847893a889aaee2ad/src/Pulsar.Client/Internal/ClientCnx.fs#L119
> > >
> > > Scala:
> > > All appear to wrap the Pulsar Java client (or don't have a
> > > clientVersion string in their code base).
> > >
> > > Thanks,
> > > Michael
> > >
> > > On Thu, Feb 23, 2023 at 11:39 AM Michael Marshall <mm...@apache.org> wrote:
> > > >
> > > > > A better solution is that we could allow the
> > > > > user to specify the suffix of the client version.
> > > >
> > > > I don't think we need to make it configurable because the library
> > > > owner, not the user, has full control when creating the Connect
> > > > command.
> > > >
> > > > We can update our protocol spec to recommend appropriate usage and of the field.
> > > >
> > > > Thanks,
> > > > Michael
> > > >
> > > > On Thu, Feb 23, 2023 at 2:51 AM Zike Yang <zi...@apache.org> wrote:
> > > > >
> > > > > > But I'm wondering if it's possible to support configuring the client
> > > > > version string by users?
> > > > >
> > > > > I think it's possible. A better solution is that we could allow the
> > > > > user to specify the suffix of the client version. For example, by
> > > > > adding a new configuration like `clientVersionSuffix` to the
> > > > > ClientConffiguration, the client_version would be like
> > > > > `java-3.0.0-forked-v1.0.0`. In this way, we could always remain the
> > > > > part `java-3.0.0`. But of course, we have no control over the behavior
> > > > > of third-party clients.
> > > > >
> > > > >
> > > > > Thanks,
> > > > > Zike Yang
> > > > >
> > > > > On Wed, Feb 22, 2023 at 5:52 AM Michael Marshall <mm...@apache.org> wrote:
> > > > > >
> > > > > > +1 - I think this is a helpful change. We already do this in the Java
> > > > > > Admin http client when we set the user agent [0].
> > > > > >
> > > > > > > But I'm wondering if it's possible to support configuring the client
> > > > > > > version string by users?
> > > > > >
> > > > > > This is always the case because we support third party clients. In my
> > > > > > view, this field is primarily helpful for troubleshooting and for
> > > > > > getting weakly trusted stats on usage in a given cluster. Operators
> > > > > > could even use this information to determine and alert if any clients
> > > > > > are connecting with known vulnerabilities.
> > > > > >
> > > > > > Thanks,
> > > > > > Michael
> > > > > >
> > > > > > [0] https://github.com/apache/pulsar/blob/d8569cd4ec6da14f8b2b9338db1ed2f6a3eacf0a/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java#L105
> > > > > >
> > > > > > On Tue, Feb 21, 2023 at 9:50 AM Yunze Xu <yz...@streamnative.io.invalid> wrote:
> > > > > > >
> > > > > > > +1
> > > > > > >
> > > > > > > But I'm wondering if it's possible to support configuring the client
> > > > > > > version string by users? For example, if users forked their own
> > > > > > > client, they might want to differ the client version with the official
> > > > > > > client. The disadvantage could be that users can configure any version
> > > > > > > string as they want.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Yunze
> > > > > > >
> > > > > > > On Tue, Feb 21, 2023 at 7:23 PM Enrico Olivelli <eo...@gmail.com> wrote:
> > > > > > > >
> > > > > > > > +1
> > > > > > > >
> > > > > > > > Enrico
> > > > > > > >
> > > > > > > > Il giorno mar 21 feb 2023 alle ore 10:23 Baodi Shi <ba...@apache.org>
> > > > > > > > ha scritto:
> > > > > > > > >
> > > > > > > > >  +1, Good idea.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > > Baodi Shi
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 在 2023年2月21日 15:24:45 上，avinash kala <av...@gmail.com> 写道：
> > > > > > > > >
> > > > > > > > > > +1, sounds good.
> > > > > > > > > >
> > > > > > > > > > On Tue, Feb 21, 2023, 12:39 PM Zixuan Liu <no...@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > +1, good idea!
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > >
> > > > > > > > > > Zixuan
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Zike Yang <zi...@apache.org> 于2023年2月21日周二 11:33写道：
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Hi, all
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Currently, the Pulsar broker uses the field `client_version` to get
> > > > > > > > > >
> > > > > > > > > > > the version of the client. The client will send the client_version to
> > > > > > > > > >
> > > > > > > > > > > the broker through `CommandConnect` [0] during the connect or
> > > > > > > > > >
> > > > > > > > > > > `CommandAuthResponse ` [1] during the auth challenge. We could get the
> > > > > > > > > >
> > > > > > > > > > > client_version from the admin using `pulsar-admin topics stats xxx`
> > > > > > > > > >
> > > > > > > > > > > [2].
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > But the `client_version ` only shows the version information. And
> > > > > > > > > >
> > > > > > > > > > > clients in different languages use their own separate version numbers.
> > > > > > > > > >
> > > > > > > > > > > This can lead to conflict. For example, the java client 2.10.2 uses
> > > > > > > > > >
> > > > > > > > > > > the same value `2.10.2` as the C++ client 2.10.2. And C++ client 3.0.0
> > > > > > > > > >
> > > > > > > > > > > may conflict with the future java client 3.0.0. The information of
> > > > > > > > > >
> > > > > > > > > > > `client_version` is incomplete. We could not determine what
> > > > > > > > > >
> > > > > > > > > > > language(or specific library) the client uses. This raises
> > > > > > > > > >
> > > > > > > > > > > inconvenience for debugging.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > I would like to propose adding the client language information to the
> > > > > > > > > >
> > > > > > > > > > > `client_version`. Like:
> > > > > > > > > >
> > > > > > > > > > > * `java-2.11.1` for Java client 2.11
> > > > > > > > > >
> > > > > > > > > > > * `cpp-3.1.2` for C++ client 3.1.2
> > > > > > > > > >
> > > > > > > > > > > * `go-0.9.0` for go client 0.9.0
> > > > > > > > > >
> > > > > > > > > > > We can easily do that because the `client_version` is a string type.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > In addition, the Nodejs client and Python client all use the
> > > > > > > > > >
> > > > > > > > > > > client_version of C++ client they depend on. So it will use `3.1.2` as
> > > > > > > > > >
> > > > > > > > > > > the client_verion in Nodejs client 1.8.0. This is incorrect, and I
> > > > > > > > > >
> > > > > > > > > > > think we need to fix them. We don't need to print the C++ client
> > > > > > > > > >
> > > > > > > > > > > version because we can get that information if we know the Nodejs or
> > > > > > > > > >
> > > > > > > > > > > Python client version.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > What do you think? Please share your thoughts if you have any ideas.
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > [0]
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > https://github.com/apache/pulsar/blob/e0b50c9ec5f12d0fb8275f235d8ac00e87a9099e/pulsar-common/src/main/proto/PulsarApi.proto#L269
> > > > > > > > > >
> > > > > > > > > > > [1]
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > https://github.com/apache/pulsar/blob/e0b50c9ec5f12d0fb8275f235d8ac00e87a9099e/pulsar-common/src/main/proto/PulsarApi.proto#L309
> > > > > > > > > >
> > > > > > > > > > > [2] https://pulsar.apache.org/docs/next/admin-api-topics/#get-stats
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Thanks,
> > > > > > > > > >
> > > > > > > > > > > Zike Yang
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >