Hi Zike and Michael, I agree that we can move this method to `ClientBuilderImpl`. Generally users should not set this description, the client version string is used to identify a specific implementation of the Pulsar API. If there is a strong demand to configure the description for the API layer (`PulsarClient`), we can open a new PIP later.
So I changed the PIP to add this method for `ClientBuilderImpl`. And still, there is a length limit for it. Thanks, Yunze On Mon, Mar 13, 2023 at 3:47 PM Zike Yang <z...@apache.org> wrote: > > > I'd rather use the "description" term, which indicates the > client version has extra description in addition to the client version > string. > > The `description` term and 64 length limit all make sense to me. > > > Is there any way we can avoid giving regular users easy access to this > field via the ClientBuilder while still letting libraries add their > own suffix? > > I think we need to consider this problem. Is it possible to expose > this field to the `ClientBuilderImpl` instead of the `ClientBuilder`? > This will make users not easy to access this field. The `description` > still seems confusing for the user. Are there any strong cases where > the field `description` makes sense to the general user and not just > the library developer? > > Thanks, > Zike Yang > > On Mon, Mar 13, 2023 at 9:31 AM Yunze Xu <y...@streamnative.io.invalid> wrote: > > > > > I think we > > > should consider putting a character limit on the field to prevent > > > descriptions that are too long. > > > > Good suggestion. A long description string is unnecessary and could be > > used as malicious code. What do you think of limiting the length to > > 64? I think it's long enough. > > > > Thanks, > > Yunze > > > > On Fri, Mar 10, 2023 at 12:52 PM Michael Marshall <mmarsh...@apache.org> > > wrote: > > > > > > Great discussion. Derivative clients are an important consideration > > > for our discussion around capturing the version information. > > > > > > Is there any way we can avoid giving regular users easy access to this > > > field via the ClientBuilder while still letting libraries add their > > > own suffix? We cannot prevent a custom library from serializing > > > whatever they would like in the field, but making this field easily > > > available to application code could be confusing and might decrease > > > the value of the version information. In my mind, the goal of the > > > version string is to give a weak signal that helps operators debug > > > client related issues. > > > > > > If we do leave this field exposed to the application code, I think we > > > should consider putting a character limit on the field to prevent > > > descriptions that are too long. > > > > > > Thanks, > > > Michael > > > > > > On Thu, Mar 9, 2023 at 8:28 PM Yunze Xu <y...@streamnative.io.invalid> > > > wrote: > > > > > > > > I've updated this proposal to retain the original client version > > > > string. I'd rather use the "description" term, which indicates the > > > > client version has extra description in addition to the client version > > > > string. > > > > > > > > Thanks, > > > > Yunze > > > > > > > > On Fri, Mar 10, 2023 at 10:11 AM Yunze Xu <y...@streamnative.io> wrote: > > > > > > > > > > Hi Zike, > > > > > > > > > > Good suggestion. I agree with this approach. Maybe we can name it as > > > > > `subVersionString` to indicate that? > > > > > > > > > > Thanks, > > > > > Yunze > > > > > > > > > > On Thu, Mar 9, 2023 at 3:14 PM Zike Yang <z...@apache.org> wrote: > > > > > > > > > > > > Hi Yunze, > > > > > > > > > > > > > I have changed this proposal to just add a config to > > > > > > > `ClientBuilder`. > > > > > > > > > > > > I propose to add a field named `clientVersionSuffix` rather than the > > > > > > `clientVersion`. As I said before: > > > > > > https://lists.apache.org/thread/g0128l85fkcmw4821188mjjznxbo4lhd > > > > > > > > > > > > This is helpful for debugging. Especially for the case of the Nodejs > > > > > > client in which users can compile the C++ client on their own. This > > > > > > way, we can know exactly which underlying C++ client version the > > > > > > user > > > > > > uses. > > > > > > > > > > > > Thanks, > > > > > > Zike Yang > > > > > > > > > > > > On Wed, Mar 8, 2023 at 5:17 PM Yunze Xu > > > > > > <y...@streamnative.io.invalid> wrote: > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > I have changed this proposal to just add a config to > > > > > > > `ClientBuilder`. > > > > > > > And here is the demo implementation: > > > > > > > https://github.com/BewareMyPower/pulsar/pull/21/files > > > > > > > > > > > > > > PTAL again. > > > > > > > > > > > > > > Thanks, > > > > > > > Yunze > > > > > > > > > > > > > > On Sat, Mar 4, 2023 at 10:39 PM Yunze Xu <y...@streamnative.io> > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi Enrico, > > > > > > > > > > > > > > > > Thanks for your suggestion. It makes sense to me. I will think > > > > > > > > again > > > > > > > > and modify this proposal. > > > > > > > > > > > > > > > > Hi Tison, > > > > > > > > > > > > > > > > I mentioned the C++ client because the initial motivation is to > > > > > > > > solve > > > > > > > > the issue for the Python client and Node.js client. But after > > > > > > > > thinking > > > > > > > > for a while, I believe it's more general for clients of other > > > > > > > > languages, including Java. And this proposal is only for the > > > > > > > > Java > > > > > > > > client. > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Yunze > > > > > > > > > > > > > > > > On Sat, Mar 4, 2023 at 1:42 PM tison <wander4...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > I agree with Enrico that it's better to have a config option. > > > > > > > > > > > > > > > > > > Also, we cannot simply replace the PulsarVersion call with the > > > > > > > > > DynamicPulsarVersion call because the client version string > > > > > > > > > is now > > > > > > > > > constructed as: > > > > > > > > > > > > > > > > > > String.format("Pulsar-Java-v%s", PulsarVersion.getVersion()) > > > > > > > > > > > > > > > > > > It's a config of client version string, not pulsar version. > > > > > > > > > > > > > > > > > > Moreover, in your proposal, you mention the case of client > > > > > > > > > c++ at first, > > > > > > > > > but don't talk about it later. Is the scope of this proposal > > > > > > > > > in the Java > > > > > > > > > client only? > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > tison. > > > > > > > > > > > > > > > > > > > > > > > > > > > Enrico Olivelli <eolive...@gmail.com> 于2023年3月4日周六 06:38写道： > > > > > > > > > > > > > > > > > > > Yunze, > > > > > > > > > > > > > > > > > > > > Il Ven 3 Mar 2023, 12:31 Yunze Xu > > > > > > > > > > <y...@streamnative.io.invalid> ha > > > > > > > > > > scritto: > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > Based on the previous discussion [1], I created a > > > > > > > > > > > proposal to support > > > > > > > > > > > configuring client version at SDK level: > > > > > > > > > > > https://github.com/apache/pulsar/issues/19705 > > > > > > > > > > > > > > > > > > > > > > I've added more explanations in the motivation part, > > > > > > > > > > > let's use this > > > > > > > > > > > PIP as a subsequent discussion of [1]. > > > > > > > > > > > > > > > > > > > > > > BTW, there is a PR [2] in the pulsar-client-cpp repo > > > > > > > > > > > because the > > > > > > > > > > > motivation is more meaningful for the C++ client. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I understand well this problem, we have it for the cited > > > > > > > > > > clients but I also > > > > > > > > > > see the same issue for other libraries based on the Java > > > > > > > > > > client, like the > > > > > > > > > > official Apache Pulsar Reactive client. > > > > > > > > > > > > > > > > > > > > I also see this problem in Startlight for JMS that is a JMS > > > > > > > > > > client for > > > > > > > > > > Pulsar that is based on the Java client. > > > > > > > > > > > > > > > > > > > > While I agree on the problem and on the solution I think > > > > > > > > > > that a static > > > > > > > > > > field is not enough, we have some problems: > > > > > > > > > > > > > > > > > > > > 1) there may be multiple usages of the Java client in the > > > > > > > > > > same JVM, and you > > > > > > > > > > want each client to report correctly its version > > > > > > > > > > > > > > > > > > > > 2) we would need to use the Java security Manager in order > > > > > > > > > > to prevent > > > > > > > > > > malicious code to modify the version or some other > > > > > > > > > > mechanism to prevent > > > > > > > > > > overriding the version. > > > > > > > > > > > > > > > > > > > > I believe that in the case of the Java client is is easier > > > > > > > > > > to add a > > > > > > > > > > configuration entry to the Pulsar Client Configuration. > > > > > > > > > > That would become a > > > > > > > > > > field in the JavaClient. So each instance can declare its > > > > > > > > > > version and also > > > > > > > > > > malicious code won't be able ti easily tweak the version > > > > > > > > > > (because it won't > > > > > > > > > > be a simple static method call) > > > > > > > > > > > > > > > > > > > > Enrico > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > https://lists.apache.org/thread/n59k537fhthjnzkfxtc2p4zk4l0cv3mp > > > > > > > > > > > [2] https://github.com/apache/pulsar-client-cpp/pull/208 > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > >
