I agree with that. And looking at the MetadataResponse fields it seems there has been some feature creep already here. Does the client use rack information, for example?
I guess one could do this either by introducing a new leaner message pair, used specifically enable client operation, and use MetadataRequest/MetadataResponse for describeCluster() or one could shrink MetadataRequest/MetadataResponse and introduce a new more fully featured message pair for the other stuff. I would be happy to spend some time looking into implementing this if there is an interest. /noa > On 5 Nov 2019, at 15:43, Gwen Shapira <g...@confluent.io> wrote: > > It isn't just about saving space. It increases complexity to default to > always sharing a bit of information that is really only needed in a single > use-case. > We avoid doing this as a matter of good protocol design. > Arguably, this should not really piggyback on cluster metadata at all, > since the usage is so different. > > On Tue, Nov 5, 2019, 7:29 AM Noa Resare <n...@resare.com> wrote: > >> It would certainly be possible to have the field be optional and only >> include it if some flag is set in the DescribeClusterOptions instance >> passed to Admin.describeCluster() that in turn would translate to a boolean >> in MetadataRequest indicating that we are asking for this piece of >> information. >> >> I’m not entirely sure that this extra complexity would be worth it for the >> modestly smaller response size, in a message that already contains things >> like the hostname and rack identifier per broker. >> >> /noa >> >>> On 4 Nov 2019, at 14:45, Gwen Shapira <g...@confluent.io> wrote: >>> >>> Cluster metadata is sent to clients on a very regular basis. Adding >>> start-time there seems quite repetitive. Especially considering that >>> this information is only useful in very specific cases. >>> >>> Can we add this capability to the Admin API in a way that won't impact >>> normal client workflow? >>> >>> On Mon, Nov 4, 2019 at 4:05 AM Noa Resare <n...@resare.com> wrote: >>>> >>>> Thank you for the feedback, Stanislav! >>>> >>>> I agree that it would be good to harmonise the naming, and >> start-time-ms definitely more descriptive. >>>> >>>> I have updated the proposal to reflect this, and also added the updated >> json RPC changes. Please have a look. >>>> >>>> /noa >>>> >>>>> On 1 Nov 2019, at 09:13, Stanislav Kozlovski <stanis...@confluent.io> >> wrote: >>>>> >>>>> Hey Noa, >>>>> >>>>> KIP-436 added a JMX metric in Kafka for this exact use-case, called >>>>> `start-time-ms`. Perhaps it would be useful to name this public >> interface >>>>> in the same way for consistency. >>>>> >>>>> Could you update the KIP to include the specific RPC changes regarding >> the >>>>> metadata request/responses? Here is a recent example of how to portray >> the >>>>> changes - >>>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-525+-+Return+topic+metadata+and+configs+in+CreateTopics+response >>>>> >>>>> Thanks, >>>>> Stanislav! >>>>> >>>>> On Mon, Oct 14, 2019 at 2:46 PM Noa Resare <n...@resare.com> wrote: >>>>> >>>>>> We are in the process of migrating the pieces of automation that >> currently >>>>>> reads and modifies zookeeper state to use the Admin API. >>>>>> >>>>>> One of the things that we miss doing this is access to the start time >> of >>>>>> brokers in a cluster which is used by our automation doing rolling >>>>>> restarts. We currently read this from the timestamp field from the >>>>>> epehmeral broker znodes in zookeeper. To address this limitation, I >> have >>>>>> authored KIP-536, that proposes adding a timestamp field to the Node >> class >>>>>> that the AdminClient.describeCluster() method returns. >>>>>> >>>>>> >>>>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-536%3A+Propagate+broker+timestamp+to+Admin+API >>>>>> < >>>>>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-536:+Propagate+broker+timestamp+to+Admin+API >>>>>>> >>>>>> >>>>>> Any and all feedback is most welcome >>>>>> >>>>>> cheers >>>>>> noa >>>>> >>>>> >>>>> >>>>> -- >>>>> Best, >>>>> Stanislav >>>> >> >>
