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
