RE: RE: Re: Re: RE: Re: RE: Re: [DISCUSS] KIP-885: Expose Broker's Name and Version to Clients Skip to end of metadata
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 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 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
RE: Re: Re: RE: Re: RE: Re: [DISCUSS] KIP-885: Expose Broker's Name and Version to Clients Skip to end of metadata
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 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 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.
Re: Re: RE: Re: RE: Re: [DISCUSS] KIP-885: Expose Broker's Name and Version to Clients Skip to end of metadata
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 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 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 > > > > > 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: > > > > > >
RE: Re: RE: Re: RE: Re: [DISCUSS] KIP-885: Expose Broker's Name and Version to Clients Skip to end of metadata
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 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 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
