Re: [VOTE] KIP-740: Use TaskId instead of String for the taskId field in TaskMetadata

[Walker Carlson <wc...@confluent.io.INVALID>]

I am making a minor update to the KIP renaming the TaskId#namedTopology
method to TaskId#topologyName as it returns a string for the name not an
object. This has approval to make it into 3.0 already.
https://github.com/apache/kafka/pull/11192

best,
Walker

On Thu, May 20, 2021 at 4:58 PM Sophie Blee-Goldman
<so...@confluent.io.invalid> wrote:

> 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 <vv...@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 <wa...@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
> > > > > <so...@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 <
> > sophie@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 <
> > vvcephei@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 <
> > > > > > > > sophie@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 <gu...@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 <
> > wangguoz@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
> > > > > > > > > > > <wc...@confluent.io.invalid> wrote:
> > > > > > > > > > >
> > > > > > > > > > > > +1 from me! (non-binding)
> > > > > > > > > > > >
> > > > > > > > > > > > Walker
> > > > > > > > > > > >
> > > > > > > > > > > > On Thu, May 13, 2021 at 1:53 PM Sophie Blee-Goldman
> > > > > > > > > > > > <so...@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
> > > > >
> > > >
> > > >
> >
> >
> >
>