SGTM for going back to encoding the full names too --- flexibility wins here, and if users do hit limits on bytes, probably they'd consider giving some shorter names anyways :)
Guozhang On Wed, Mar 17, 2021 at 11:25 AM Levani Kokhreidze <levani.co...@gmail.com> wrote: > 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 > >> > >> > > -- -- Guozhang
