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 > > > > > > >
