Thanks for the updates. The latest KIP LGTM. On Fri, Sep 20, 2024 at 8:38 AM Gantigmaa Selenge <gsele...@redhat.com> wrote: > > 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é > > > >
-- -José
