Thanks Bruno, fixed. Definitely a leftover from one of the many iterations of this KIP.
Guozhang, Thanks for pointing out the change in TaskMetadata constructor, I definitely agree to just swap out the constructor since that's not really the useful part of this API. I'm more on the fence when it comes to the taskId() getter -- personally of course I would prefer to just change it directly than go through this deprecation cycle, but unlike the constructor it seems likely that some users are/have been relying on the taskId() method. I think we can conclude the voting on this KIP at last, with four +1 (binding) votes from Guozhang, John, Bruno, and myself, and one +1 (non-binding) from Walker. Appreciate the discussion and the questions it has raised. Though I was not expecting this to be so "complicated", I feel good that we'll be leaving the code and API in a better place and opened the door for potential further improvements to come. -Sophie On Thu, May 20, 2021 at 8:00 AM John Roesler <vvcep...@apache.org> wrote: > Thanks for the updates, Sophie, > > I'm +1 (binding) > > -John > > On Thu, 2021-05-20 at 12:54 +0200, Bruno Cadonna wrote: > > Thanks for the KIP, Sophie! > > > > +1 (binding) > > > > Note: > > The sentence in the KIP seems to need some corrections: > > > > "To migrate from the String to TaskIdInfo in TaskMetadata, we'll need to > > deprecate the existing taskId() getter method and add a TaskId() getter > > in its place." > > > > I guess you wanted to write: > > > > "To migrate from the String to *TaskId* in TaskMetadata, we'll need to > > deprecate the existing taskId() getter method and add a *getTaskId()* > > getter in its place." > > > > > > Best, > > Bruno > > > > On 20.05.21 08:18, Guozhang Wang wrote: > > > A quick note: since we changed the constructor of TaskMetadata as well > in > > > the PR, we'd need to add that in the KIP wiki as well. Personally I > think > > > it is okay to just replace the constructor as you did in the PR rather > than > > > adding/deprecating --- I would even advocate for replacing the `taskId` > > > function with the new return type without introducing a new one with > > > different name, but I knew since this is not favored by most people :). > > > > > > On Wed, May 19, 2021 at 11:01 PM Guozhang Wang <wangg...@gmail.com> > wrote: > > > > > > > Thanks Sophie, I like the current proposal better compared to adding > a new > > > > TaskInfo class. +1 ! > > > > > > > > Guozhang > > > > > > > > On Wed, May 19, 2021 at 4:58 PM Sophie Blee-Goldman > > > > <sop...@confluent.io.invalid> wrote: > > > > > > > > > Just a friendly ping to please check out the finalized proposal of > the KIP > > > > > and (re)cast your votes > > > > > > > > > > Thanks! > > > > > Sophie > > > > > > > > > > On Sun, May 16, 2021 at 7:28 PM Sophie Blee-Goldman < > sop...@confluent.io> > > > > > wrote: > > > > > > > > > > > Thanks John. I have moved the discussion over to a [DISCUSS] > thread, > > > > > where > > > > > > it should have been taking place all > > > > > > along. I'll officially kick off the vote again, but since this > KIP has > > > > > > been through a significant overhauled since it's initial > > > > > > proposal, the previous votes cast will be invalidated. Please > make a > > > > > pass > > > > > > on the latest KIP and (re)cast your vote. > > > > > > > > > > > > If you have any concerns or comments beyond just small > questions, please > > > > > > take them to the discussion thread. > > > > > > > > > > > > Thanks! > > > > > > Sophie > > > > > > > > > > > > On Fri, May 14, 2021 at 10:12 AM John Roesler < > vvcep...@apache.org> > > > > > wrote: > > > > > > > > > > > > > Thanks for these updates, Sophie, > > > > > > > > > > > > > > Unfortunately, I have some minor suggestions: > > > > > > > > > > > > > > 1. "Topic Group" is a vestigial term from the early days of > > > > > > > the codebase. We call a "topic group" a "subtopology" in the > > > > > > > public interface now (although "topic group" is still used > > > > > > > internally some places). For user-facing consistency, we > > > > > > > should also use "subtopologyId" in your proposal. > > > > > > > > > > > > > > 2. I'm wondering if it's really necessary to introduce this > > > > > > > interface at all. I think your motivation is to be able to > > > > > > > get the subtopologyId and partition via TaskMetadata, right? > > > > > > > Why not just add those methods to TaskMetadata? Stepping > > > > > > > back, the concept of metadata about an identifier is a bit > > > > > > > elaborate. > > > > > > > > > > > > > > Sorry for thrashing what you were hoping would be a quick, > > > > > > > uncontroversial KIP. > > > > > > > > > > > > > > Thanks for your consideration, > > > > > > > John > > > > > > > > > > > > > > On Thu, 2021-05-13 at 19:35 -0700, Sophie Blee-Goldman > > > > > > > wrote: > > > > > > > > One last update: we will not actually remove the existing > > > > > > > > o.a.k.streams.processor.TaskId class, but only > > > > > > > > deprecate it, along with any methods that returned it (ie the > > > > > getters on > > > > > > > > ProcessorContext and StateStoreContext) > > > > > > > > > > > > > > > > Internally, everything will still be converted to use the new > > > > > internal > > > > > > > > TaskId class, and public TaskIdMetadata interface, > > > > > > > > where appropriate. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, May 13, 2021 at 6:42 PM Sophie Blee-Goldman < > > > > > > > sop...@confluent.io> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Thanks all. I updated the KIP slightly since there is some > > > > > ambiguity > > > > > > > > > around whether the existing TaskId class is > > > > > > > > > currently part of the public API or not. To settle the > matter, I > > > > > have > > > > > > > > > introduced a new public TaskId interface that > > > > > > > > > exposes the metadata, and moved the existing TaskId class > to the > > > > > > > internals > > > > > > > > > package. The KIP < > https://cwiki.apache.org/confluence/x/vYTOCg> > > > > > has > > > > > > > been > > > > > > > > > updated > > > > > > > > > with the proposed API changes. > > > > > > > > > > > > > > > > > > @Guozhang Wang <guozh...@confluent.io> : I decided to > leave this > > > > > new > > > > > > > > > TaskId interface in o.a.k.streams.processor since that's > where the > > > > > > > > > TaskMetadata class is, along with the other related > metadata > > > > > classes > > > > > > > (eg > > > > > > > > > ThreadMetadata). I do agree it makes > > > > > > > > > more sense for them to be under o.a.k.streams, but I'd > rather leave > > > > > > > them > > > > > > > > > together for now. > > > > > > > > > > > > > > > > > > Please let me know if there are any concerns, or you want > to redact > > > > > > > your > > > > > > > > > vote :) > > > > > > > > > > > > > > > > > > -Sophie > > > > > > > > > > > > > > > > > > On Thu, May 13, 2021 at 3:11 PM Guozhang Wang < > wangg...@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > On a hindsight, maybe TaskId should not really be in > > > > > > > > > > `org.apache.kafka.streams.processor` but rather just in > > > > > > > `o.a.k.streams`, > > > > > > > > > > but maybe not worth pulling it up now :) > > > > > > > > > > > > > > > > > > > > Guozhang > > > > > > > > > > > > > > > > > > > > On Thu, May 13, 2021 at 1:58 PM Walker Carlson > > > > > > > > > > <wcarl...@confluent.io.invalid> wrote: > > > > > > > > > > > > > > > > > > > > > +1 from me! (non-binding) > > > > > > > > > > > > > > > > > > > > > > Walker > > > > > > > > > > > > > > > > > > > > > > On Thu, May 13, 2021 at 1:53 PM Sophie Blee-Goldman > > > > > > > > > > > <sop...@confluent.io.invalid> wrote: > > > > > > > > > > > > > > > > > > > > > > > Hey all, > > > > > > > > > > > > > > > > > > > > > > > > I'm just going to take this KIP straight to a vote > since it > > > > > > > should be > > > > > > > > > > a > > > > > > > > > > > > trivial and uncontroversial change. Of course please > raise > > > > > any > > > > > > > > > > concerns > > > > > > > > > > > > should they come up, and I can take things to a > DISCUSS > > > > > thread. > > > > > > > > > > > > > > > > > > > > > > > > The KIP is a simple change to move from String to > TaskId for > > > > > the > > > > > > > > > > taskID > > > > > > > > > > > > field of TaskMetadata. > > > > > > > > > > > > > > > > > > > > > > > > KIP-740: Use TaskId instead of String for the taskId > field in > > > > > > > > > > > TaskMetadata > > > > > > > > > > > > < > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-740%3A+Use+TaskId+instead+of+String+for+the+taskId+field+in+TaskMetadata > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Cheers, > > > > > > > > > > > > Sophie > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > -- Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > -- Guozhang > > > > > > > > > > > > >
