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