Thank you Jose. > I think that we should always output the RACK column even if all of the nodes do not contain rack information. This would make it easier for Kafka operators that develop scripts which use and parse CLI outputs. What do you think?
That makes sense to me, so I updated the KIP and the CLI output will always include the rack information. > Sounds good to me. Should we also add a ROLES column to the output where the possible values are "broker" and "controller"? --bootstrap-server will always set that column to "broker" while --bootstrap-controller will always set that column to "controller". This would reinforce to users that are less familiar with the implementation of the role of the returned endpoints. 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. > While thinking about the comment above, I realized that something important to note is that this RPC and command return the node _endpoints_ and not necessarily the nodes. For example, what happens if some of the brokers (1 and 2) have the advertised listeners (A, B and C) and some of the brokers (3 and 4) have the advertised listeners (A, B) yet the user uses a bootstrap server endpoint that maps to the listener C? The current implementation would only return brokers 1 and 2 since those are the only brokers that have listener C. Are you planning to keep this implementation? I think we have to, to keep it compatible with the previous implementation. If so, maybe we can make this clearer by calling the command "list-endpoints" instead of "list-nodes". What do you think? This is a good point. It seems like nodes are retrieved from either BrokerRegistration or ControllerRegistration but then they are filtered based on the listener used for the bootstrap. I would have expected the describeCluster API to return all the nodes that are registered, not just the ones matching the listener because the result contains Nodes, rather than endpoints. However, I'm not sure this implementation should change with this KIP. So I will change the new command to list-endpoints from list-nodes to make it clearer with the CLI at least. Regards, Gantigmaa On Tue, Sep 10, 2024 at 4:18 PM José Armando García Sancio <jsan...@confluent.io.invalid> wrote: > Gantigmaa, thanks for the changes, they look good to me in general, I > have some UX questions. > > The KIP mentions the following: > > Also if there is no rack information for any of the nodes, the RACK > column will be omitted from the output. > > What will the tool output for nodes that don't have a rack when other > nodes contain rack information? > > I think that we should always output the RACK column even if all of > the nodes do not contain rack information. This would make it easier > for Kafka operators that develop scripts which use and parse CLI > outputs. What do you think? > > > I think we can change the command to "list-nodes" and > output either controller nodes or broker nodes depending on the bootstrap > argument. > > Sounds good to me. Should we also add a ROLES column to the output > where the possible values are "broker" and "controller"? > --bootstrap-server will always set that column to "broker" while > --bootstrap-controller will always set that column to "controller". > This would reinforce to users that are less familiar with the > implementation of the role of the returned endpoints. > > While thinking about the comment above, I realized that something > important to note is that this RPC and command return the node > _endpoints_ and not necessarily the nodes. For example, what happens > if some of the brokers (1 and 2) have the advertised listeners (A, B > and C) and some of the brokers (3 and 4) have the advertised listeners > (A, B) yet the user uses a bootstrap server endpoint that maps to the > listener C? The current implementation would only return brokers 1 and > 2 since those are the only brokers that have listener C. > > Are you planning to keep this implementation? I think we have to, to > keep it compatible with the previous implementation. If so, maybe we > can make this clearer by calling the command "list-endpoints" instead > of "list-nodes". What do you think? > > Thanks > -- > -José > >
