Hey Levani, thanks for the KIP! This looks great. A few comments/questions about the "task.assignment.rack.awareness" config: since this is used to determine the encoding of the client tags, we should make sure to specify that this list must be identical in contents and order across all clients in the application. Unfortunately there doesn't seem to be a good way to actually enforce this, so we should call it out in the config doc string at the least.
On that note, should it be possible for users to upgrade or evolve their tags over time? For example if a user wants to leverage this feature for an existing app, or add new tags to an app that already has some configured. I think we would need to either enforce that you can only add new tags to the end but never remove/change/reorder the existing ones, or else adopt a similar strategy as to version probing and force all clients to remain on the old protocol until everyone in the group has been updated to use the new tags. It's fine with me if you'd prefer to leave this out of scope for the time being, as long as we design this to be forwards-compatible as best we can. Have you considered adding a "ClientTagsVersion" to the SubscriptionInfo so that we're set up to extend this feature in the future? In my experience so far, any time we *don't* version a protocol like this we end up regretting it later. Whatever you decide, it should be documented clearly -- in the KIP and in the actual docs, ie upgrade guide and/or the config doc string -- so that users know whether they can ever change the client tags on a running application or not. (I think this is hinted at in the KIP, but not called out explicitly) By the way, I feel the "task.assignment.rack.awareness" config should have the words "clients" and/or "tags" somewhere in it, otherwise it's a bit unclear what it actually means. And maybe it should specify that it applies to standby task placement only? Obviously we don't need to cover every possible detail in the config name alone, but it could be a little more specific. What about "standby.task.rack.aware.assignment.tags" or something like that? On Mon, Mar 15, 2021 at 2:12 PM Levani Kokhreidze <levani.co...@gmail.com> wrote: > Hello all, > > Bumping this thread as we are one binding vote short accepting this KIP. > Please let me know if you have any extra concerns and/or suggestions. > > Regards, > Levani > > > On 12. Mar 2021, at 13:14, Levani Kokhreidze <levani.co...@gmail.com> > wrote: > > > > Hi Guozhang, > > > > Thanks for the feedback. I think it makes sense. > > I updated the KIP with your proposal [1], it’s a nice optimisation. > > I do agree that having the same configuration across Kafka Streams > instances is the reasonable requirement. > > > > Best, > > Levani > > > > [1] - > https://cwiki.apache.org/confluence/display/KAFKA/KIP-708%3A+Rack+awareness+for+Kafka+Streams > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-708%3A+Rack+awareness+for+Kafka+Streams > > > > > > > >> On 12. Mar 2021, at 03:36, Guozhang Wang <wangg...@gmail.com <mailto: > wangg...@gmail.com>> wrote: > >> > >> Hello Levani, > >> > >> Thanks for the great write-up! I think this proposal makes sense, > though I > >> have one minor suggestion regarding the protocol format change: note the > >> subscription info is part of the group metadata message that we need to > >> write into the internal topic, and hence it's always better if we can > save > >> on the number of bytes written there. For this, I'm wondering if we can > >> encode the key part instead of writing raw bytes based on the > >> configurations, i.e.: > >> > >> 1. streams will look at the `task.assignment.rack.awareness` values, and > >> encode them in a deterministic manner, e.g. in your example zone = 0, > >> cluster = 1. This assumes that all instances will configure this value > in > >> the same way and then with a deterministic manner all instances will > have > >> the same encodings, which I think is a reasonable requirement. > >> 2. the sent protocol would be "key => short, value => bytes" instead. > >> > >> > >> WDYT? > >> > >> Otherwise, I'm +1 on the KIP! > >> > >> Guozhang > >> > >> > >> > >> > >> On Thu, Mar 11, 2021 at 8:29 AM John Roesler <vvcep...@apache.org > <mailto:vvcep...@apache.org>> wrote: > >> > >>> Thanks for the KIP! > >>> > >>> I'm +1 (binding) > >>> > >>> -John > >>> > >>> On Wed, 2021-03-10 at 13:13 +0200, Levani Kokhreidze wrote: > >>>> Hello all, > >>>> > >>>> I’d like to start the voting on KIP-708 [1] > >>>> > >>>> Best, > >>>> Levani > >>>> > >>>> [1] - > >>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-708%3A+Rack+awareness+for+Kafka+Streams > < > https://cwiki.apache.org/confluence/display/KAFKA/KIP-708%3A+Rack+awareness+for+Kafka+Streams > > > >>>> > >>> > >>> > >>> > >> > >> -- > >> -- Guozhang > > > >
