Hi everyone, Thanks for reviewing the KIP again! I have updated it to address the latest comments from Colin and Jose.
Going back to Jose's earlier comment here: > Did you consider making "includeFencedBroker" field value implicitly based on the DescribeClusterRequest version and the EndpointType? For example, if the request version is 2 and the endpoint type is broker, the response will include all of the brokers and if they are fenced or unfenced. I am suggesting this because a request of endpointType controller and includeFencedBroker true doesn't make sense. In general, users of the AdminClient cannot easily discover the API versions supported by the destination node since the destination node may not be known when the admin call is made. I think admins would request fenced brokers only in certain scenarios, so perhaps it makes sense to include them only when requested via the new option. If the endpoint type is controller and the option to include fenced brokers set to true, the Admin Client can throw an "IllegalArgumentException" without sending the request to the controller. If you are happy with the updates I made, I would like to start the vote on this KIP. Thank you. Regards, Gantigmaa On Sat, Sep 14, 2024 at 9:51 PM Colin McCabe <cmcc...@apache.org> wrote: > Hi Gantigmaa Selenge, > > I think we should have a boolean in the request like "includeFenced" which > defaults to false. Then if we can't include the fenced brokers, we throw an > UnsupportedVersionException and fall back to the old behavior. > > There are two main reasons for this: > 1. if we don't do this, we're changing the behavior for old clients in an > incompatible way. They used to get only unfenced brokers, now they get both > and they have no way of telling which is which. > > 2. new clients still would like to know whether they're getting > everything, or just unfenced brokers, for informational reasons. > > Probably the command-line tool could print out something like "fenced > brokers will not be shown" in the case where it's talking to an old server. > Or the tool could have a flag like --show-fenced which fails if it can't be > done? > > best, > Colin > > > On Fri, Sep 13, 2024, at 08:28, José Armando García Sancio wrote: > > Thanks Gantigmaa. See comments below. The KIP LGTM after this. > > > > On Thu, Sep 12, 2024 at 4:50 AM Gantigmaa Selenge <gsele...@redhat.com> > wrote: > >> Will this make it more confusing for combined nodes? Users might expect > to > >> see both controller and broker under ROLES if it is a combined node. > Since > >> we would be able to output only one depending on the bootstrap option, I > >> wonder if it's better to stay consistent with the information in the > result > >> returned by the API. > > > > How about ENDPOINT-TYPE for the column name? That matches the keyword > > used in the request and better matches the semantic of the output. > > > > Also, I am okay if you want to leave it out. I thought the user may > > find it useful but if you find it confusing we can leave it out. > > > > Thanks, > > -- > > -José > >
