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