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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
