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