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