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