Hi Sophie, No worries! And thanks for taking a look. I’ve updated the KIP.
Will wait some time for any additional feedback that might arise. Best, Levani > On 17. Mar 2021, at 19:11, Sophie Blee-Goldman <sop...@confluent.io.INVALID> > wrote: > > Ah, sorry for hijacking the VOTE thread :( > > Limiting the tag length and total amount of tags specified are already part >> of the implementation I work on. Assuming that > > encoding a limited number of strings is acceptable, I think it's the most >> straightforward way to move forward. Any objections? > > > This sounds good to me -- I imagine most users probably only need a handful > of tags anyway. If someone is bumping up > against the limit and has a valid use case, we can always increase it. > > One last minor thing -- if we're going to encode the full tag names, then I > think we can leave out the "version" field from the > ClientTag struct. If we ever want to modify this struct, we should do so by > bumping the overall SubscriptionInfo protocol version. > This way we have one fewer version in the mix, and we get all the benefits > of version probing already baked in -- which > means we can modify the protocol however we like without worrying about > compatibility. For example this gives us the flexibility > to go back to some kind of encoding if need be (although I don't expect to > need to). > > If everyone else is on board with the current KIP, I'm +1 (binding) -- > thanks for the proposal Levani! > > Cheers, > Sophie > > > On Wed, Mar 17, 2021 at 7:54 AM Levani Kokhreidze <levani.co...@gmail.com> > wrote: > >> Hi Sophie and Bruno, >> >> Thanks for the questions and suggestions. >> Not sure if it's best to discuss this in the DISCUSSION thread, but I will >> write it here first, and if it needs more discussion, we can move to the >> DISCUSSION thread. >> Actually, in the implementation, I have a version field in the ClientTag >> struct. I assumed that all structs must-have versions, and it's an explicit >> requirement; therefore, I left it out of KIP (fixed). >> I'm okay with changing the name to "rack.aware.assignment.tags" (fixed). >> As for upgrade and evolving tags, good question, we must try to make it as >> flexible as possible. Good catch that with encoding changing the tags may >> be problematic, especially changing the tags' order. One other way around >> it maybe is to change the "rack.aware.assignment.tags" config in a way that >> users can specify the tag index. For instance: >> rack.aware.assignment.tags.0: cluster, rack.aware.assignment.1: zone; But >> configuration is way uglier and more complicated (and easier to get wrong). >> Limiting the tag length and total amount of tags specified are already part >> of the implementation I work on. Assuming that encoding a limited number of >> strings is acceptable, I think it's the most straightforward way to move >> forward. Any objections? >> I've updated KIP [1] with the latest discussion points and reverted the >> "encoding tag keys" part (sorry Guozhang, I haven't really thought about >> this potential edge-case, and thanks, Sophie, for catching it). >> >> I am looking forward to your feedback. >> >> Best, >> Levani >> >> [1] - >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-708%3A+Rack+awareness+for+Kafka+Streams >> >>> On 16. Mar 2021, at 23:30, Bruno Cadonna <br...@confluent.io.INVALID> >> wrote: >>> >>> Sophie >> >>
