Hi Jason & Gwen,
Sure, this would solve our use case. I do have two questions, however:
Moving from start time to uptime in theory would neatly side step the clock
skew problem,
but in practice I’m not sure it helps that much as the straightforward way of
going about
implementing this by returning (now - startTime) would break when the clock on
the broker
is adjusted. So, I think that startTime is more straight forward and doesn’t
hide the fact that
the system clock is sometimes off.
Another thing I think would be useful is to build support for requesting
multiple (or all?)
brokers in a single request at the start. Having the request hold a set of
brokerIds
and return a set of brokers in the response, adding a brokerId field to
identify which
broker a specific broker status response is associated with seems straight
forward.
I’ll start writing a proposal so that we have something concrete to discuss.
/noa
> On 13 Nov 2019, at 16:43, Jason Gustafson <ja...@confluent.io> wrote:
>
> Ni Noa,
>
> Thanks for the KIP. I agree with the concerns about Metadata bloat. In
> fact, I have wanted a BrokerStatus API for a while now. Perhaps this is a
> good excuse to introduce it. I was thinking something like this:
>
> BrokerStatusRequest => BrokerId
>
> BrokerStatusResponse =>
> Listeners => [ListenerName Host Port]
> RackId
> BrokerEpoch
> BrokerState => {ACTIVE, RECOVERING, SHUTTING_DOWN}
> UpTime
>
> What do you think?
>
> Thanks,
> Jason
>
> On Tue, Nov 5, 2019 at 12:25 PM Noa Resare <n...@resare.com> wrote:
>
>> 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
>>>>>>
>>>>
>>>>
>>
>>
