RE: RE: Re: Re: RE: Re: RE: Re: [DISCUSS] KIP-885: Expose Broker's Name and Version to Clients Skip to end of metadata

[Travis Bischel <tr...@gmail.com>]

Pinging on this, to refresh any inboxes.

On 2023/01/01 20:16:18 Travis Bischel wrote:
> To confirm, you’re now thinking that we should add name + version to every broker in the response? AFAICT, this grows the complexity of implementation a good amount. The implementation of the current proposal is just to add a configuration option and put it in the new string. Adding these fields to the metadata log seems odd (it’s not _really_ interesting to the brokers themselves) and much more technically complex. I’d still personally lean to keep these as top-level fields from only the replying broker. Is there a way we can get another opinion here?
> 
> - Travis
> 
> On 2022/12/16 16:21:59 David Jacot wrote:
> > 05/06. I also find BrokerSoftwareName and BrokerSoftwareVersion odd to
> > be honest because the RPC is supposed to describe the cluster. I was
> > also re-considering adding it per broker but this would be a little
> > more involved. It basically requires every broker to send their
> > version to the controller and the controller has to send the versions
> > to all the brokers via the metadata log. That would be the cleanest
> > approach.
> > 
> > Best,
> > David
> > 
> > On Fri, Dec 2, 2022 at 11:58 PM Travis Bischel <tr...@gmail.com> wrote:
> > >
> > > Thanks for the reply,
> > >
> > > 04. `nullable-string`, my mistake on that — this is the representation I have for nullable strings in my own DSL in franz-go. I’ve fixed that.
> > >
> > > 05. I think that ClusterSoftwareVersion and ClusterSoftwareName would be a bit odd: technically these are per-broker responses, and if a person truly does want to determine the cluster version, they need to request all brokers. It will be more difficult to explain in documentation that “even though this says cluster, it means broker”. I was also figuring that `BrokerSoftwareName` and `BrokerSoftwareVersion` immediately following the `Brokers` array had nice consistency.
> > >
> > > 06. I’ve added an AdminClient API section that adds two new methods to DescribeClusterResponse: `public String brokerSoftwareName()` and `public String brokerSoftwareVersion()`.
> > >
> > > 07. I’ve added a “Return the name and version per broker in the DescribeCluster response” rejected alternative.
> > >
> > > Let me know what you think,
> > > - Travis
> > >
> > > On 2022/12/02 17:25:37 David Jacot wrote:
> > > > Yeah, it is too late for 3.4. I have a few more comments:
> > > >
> > > > 04. `nullable-string` is not a valid type. You have to use "type":
> > > > "string", "versions": "1+", "nullableVersions": "1+".
> > > >
> > > > 05. BrokerSoftwareName/BrokerSoftwareVersion feel a bit weird in a
> > > > DescribeClusterResponse. I wonder if we should replace Broker by
> > > > Cluster. It is not 100% accurate but it is rare to not have an
> > > > homogeneous cluster.
> > > >
> > > > 06. We need to extend the java Admin client to expose those fields. We
> > > > cannot add fields to the protocol that are not used anywhere in Kafka.
> > > >
> > > > 07. Could we add in the rejected alternatives why we don't add the
> > > > name/version per broker? It is because it is not available centrally
> > > > in Kafka.
> > > >
> > > > Best,
> > > > David
> > > >
> > > > On Fri, Dec 2, 2022 at 6:03 PM Travis Bischel <tr...@gmail.com> wrote:
> > > > >
> > > > > I see now that this KIP is past the freeze deadline and will not make 3.4 — but, 3.4 thankfully will still be able to be detected by an ApiVersions response due to KIP-866. I’d like to proceed with this KIP to ensure full implementation can be agreed upon and merged by 3.5.
> > > > >
> > > > > Thanks!
> > > > > - Travis
> > > > >
> > > > > On 2022/12/02 16:40:26 Travis Bischel wrote:
> > > > > > Hi David,
> > > > > >
> > > > > > No worries for the late reply — my main worry is getting this in by Kafka 3.4 so there is no gap in detecting versions :)
> > > > > >
> > > > > > I’m +1 to adding this to DescribeCluster. I just edited the KIP to replace ApiVersions with DescribeCluster, and added the original ApiVersions idea to the list of rejected alternatives. Please take a look again and let me know what you think.
> > > > > >
> > > > > > Thank you!
> > > > > > - Travis
> > > > > >
> > > > > > On 2022/12/02 15:35:09 David Jacot wrote:
> > > > > > > Hi Travis,
> > > > > > >
> > > > > > > Please, excuse me for my late reply.
> > > > > > >
> > > > > > > 02. Yeah, I agree that it is more convenient to rely on the api
> > > > > > > versions but that does not protect us from misuages.
> > > > > > >
> > > > > > > 03. Yeah, I was about to suggest the same. Adding the information to
> > > > > > > the DescribeCluster API would work and we also have the
> > > > > > > Admin.describeCluster API to access it. We could provide the software
> > > > > > > name and version of the broker that services the request. Adding it
> > > > > > > per broker is challenging because the broker doesn't know the version
> > > > > > > of the others. If you use the API directly, you can always send it to
> > > > > > > all brokers in the cluster to get all the versions. This would also
> > > > > > > mitigate 02. because clients won't use the DescribeCluster API to gate
> > > > > > > features.
> > > > > > >
> > > > > > > Best,
> > > > > > > David
> > > > > > >
> > > > > > > On Fri, Nov 11, 2022 at 5:50 PM Travis Bischel <tr...@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Two quick mistakes to clarify:
> > > > > > > >
> > > > > > > > When I say ClusterMetadata, I mean request 60, DescribeCluster.
> > > > > > > >
> > > > > > > > Also, the email subject of this entire thread should be "[DISCUSS] KIP-885: Expose Broker's Name and Version to Clients”. I must have either accidentally pasted the “Skip to end of metadata”, or did not delete something.
> > > > > > > >
> > > > > > > > Cheers,
> > > > > > > > -Travis
> > > > > > > >
> > > > > > > > On 2022/11/11 16:45:12 Travis Bischel wrote:
> > > > > > > > > Thanks for the replies David and Magnus
> > > > > > > > >
> > > > > > > > > David:
> > > > > > > > >
> > > > > > > > > 02: From a client implementation perspective, it is easier to gate features based on the max version numbers returned per request, rather than any textual representation of a string. I’m not really envisioning a client implementation trying to match on an undefined string, especially if it’s documented as just metadata information.
> > > > > > > > >
> > > > > > > > > 03: Interesting, I may be one of the few that does query the version directly. Perhaps this can be some new information that is instead added to request 60, ClusterMetadata? The con with ClusterMetadata is that I’m interested in this information on a per-broker basis. We could add these fields per each broker in the Brokers field, though.
> > > > > > > > >
> > > > > > > > > Magnus:
> > > > > > > > >
> > > > > > > > > As far as I can see, only my franz-go client offers this ability to “guess” the version of a broker — and it’s historically done so through ApiVersions, which was the only way to do this. This feature was used in three projects by two people: my kcl project, and the formerly-known-as Kowl and Kminion projects.
> > > > > > > > >
> > > > > > > > > After reading through most of the discussion thread on KIP-35, it seems that the conversation about using a broker version string / cluster aggregate version was specifically related to having the client choose how to behave (i.e., choose what versions of requests to use). The discussion was not around having the broker version as a piece of information that a client can use in log lines or for end-user presentation purposes.
> > > > > > > > >
> > > > > > > > > It seems a bit of an misdirected worry that a client implementor may accidentally use an unstructured string field for versioning purposes, especially when another field (ApiKeys) exists for versioning purposes and is widely known. Implementing a Kafka client is quite complex and there are so many other areas an implementor can go wrong, I’m not sure that we should be worried about an unstructured and documented metadata field.
> > > > > > > > >
> > > > > > > > > "the existing ApiVersionsReq  … this information is richer than a single version string"
> > > > > > > > >
> > > > > > > > > Agree, this true for clients. However, it’s completely useless visually for end users.
> > > > > > > > >
> > > > > > > > > The reason Kminion used the version guess was two fold: to emit log a log on startup that the process was talking to Kafka v#.#, and to emit a const gauge metric for Prometheus where people could monitor external to Kafka what version each broker was running.
> > > > > > > > >
> > > > > > > > > Kowl uses the version guess to display the Kafka version the process is talking to immediately when you go to the Brokers panel. I envision that this same UI display can be added to Conduktor, even Confluent, etc.
> > > > > > > > >
> > > > > > > > > kcl uses the version guess as an extremely quick debugging utility: software engineers (not cluster administrators) might not always know what version of Kafka they are talking to, but they are trying to use a client. I often receive questions about “why isn’t this xyz thing working”, I ask for their cluster version with kcl, and then we can jump into diagnosing the problem much quicker.
> > > > > > > > >
> > > > > > > > > I think, if we focus on the persona of a cluster administrator, it’s not obvious what the need for this KIP is. For me, focusing on the perspective of an end-user of a client makes the need a bit clearer. It is not the responsibility of an end-user to manage the cluster version, but it is the responsibility of an end-user to know which version of a cluster they are talking to so that they know which fields / requests / behaviors are supported in a client
> > > > > > > > >
> > > > > > > > > All that said: I think this information is worth it and unlikely to be misused. IMO, ApiVersions is the main place to include this information, but another alternative is ClusterMetadata. Adding these fields to ClusterMetadata might be a bit awkward and may return stale information sometimes during a rolling upgrade.
> > > > > > > > >
> > > > > > > > > Curious to know your thoughts, and again thank you for the consideration and replies,
> > > > > > > > > - Travis
> > > > > > > > >
> > > > > > > > > On 2022/11/11 13:07:37 Magnus Edenhill wrote:
> > > > > > > > > > Hi Travis and thanks for the KIP, two comments below:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Den fre 11 nov. 2022 kl 13:37 skrev David Jacot <da...@gmail.com>:
> > > > > > > > > >
> > > > > > > > > > > 02: I am a bit concerned by clients that could misuse these information.
> > > > > > > > > > > For instance, one may be tempted to rely on the version to decide whether a
> > > > > > > > > > > feature is enabled or not. The api versions should remain the source of
> > > > > > > > > > > truth but we cannot enforce it with the proposed approach. That may be
> > > > > > > > > > > acceptable though.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > We proposed including this in the original ApiVersionRequest KIP-35 (waaay
> > > > > > > > > > back), but it was rejected
> > > > > > > > > > for exactly this reason; that it may(will) be misused by clients.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I would also like to question the actual end-user use of this information,
> > > > > > > > > > the existing ApiVersionsReq
> > > > > > > > > > already provides - on a detailed level - what features and functionality
> > > > > > > > > > the broker supports -
> > > > > > > > > > this information is richer than a single version string.
> > > > > > > > > >
> > > > > > > > > > The KIP says "End users can quickly check from a client if the cluster is
> > > > > > > > > > up to date" and
> > > > > > > > > > "Clients can also use the broker version in log lines so that end users can
> > > > > > > > > > quickly see
> > > > > > > > > > if a version looks about right or if something is seriously broken.":
> > > > > > > > > >
> > > > > > > > > > In my mind that's not typically the end-users role or responsibility, but
> > > > > > > > > > that of the Kafka cluster operator,
> > > > > > > > > > who'll already know the version being deployed.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > > Magnus
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Le jeu. 10 nov. 2022 à 19:10, Travis Bischel <tr...@gmail.com> a
> > > > > > > > > > > écrit :
> > > > > > > > > > >
> > > > > > > > > > > > Hi all,
> > > > > > > > > > > >
> > > > > > > > > > > > I've written a KIP to expose the BrokerSoftwareName and
> > > > > > > > > > > > BrokerSoftwareVersion to clients:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-885%3A+Expose+Broker%27s+Name+and+Version+to+Clients
> > > > > > > > > > > >
> > > > > > > > > > > > If we agree this is useful, it would be great to have this in by 3.4.
> > > > > > > > > > > >
> > > > > > > > > > > > Thank you,
> > > > > > > > > > > > - Travis
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > >
> > > >
> >