Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

Thanks Nishanth,

I've taken a look at the updated KIP and it looks good to me. I think you
can start a new VOTE thread on the current proposal.


Guozhang

On Tue, Jul 24, 2018 at 5:56 PM, Nishanth Pradeep 
wrote:

> I have updated the KIP
>  TopologyDescription+to+better+represent+Source+and+Sink+Nodes>
> .
>
> Changes to the KIP:
>
>- Removed topics() from the Public Interface and Proposed Changes
>sections.
>- Added topics() to the Deprecation plan.
>
> Thanks again for the feedback.
>
> Best,
> Nishanth Pradeep
>
> On Tue, Jul 24, 2018 at 11:21 AM Guozhang Wang  wrote:
>
> > We should not remove it immediately in the up coming 2.1 release. Usually
> > we first mark an API as deprecated, and consider removing it only after
> it
> > has been deprecated for at least one major release period.
> >
> >
> > Guozhang
> >
> > On Mon, Jul 23, 2018 at 7:40 PM, Nishanth Pradeep  >
> > wrote:
> >
> > > Sounds good to me too.
> > >
> > > As far as deprecating goes -- should the topics() method removed
> > completely
> > > or should it have a @deprecated annotation for removal in some future
> > > version?
> > >
> > > Best,
> > > Nishanth Pradeep
> > >
> > > On Sun, Jul 22, 2018 at 1:32 PM Matthias J. Sax  >
> > > wrote:
> > >
> > > > Works for me.
> > > >
> > > > On 7/22/18 9:48 AM, Guozhang Wang wrote:
> > > > > I think I can be convinced with deprecating topics() to keep API
> > > minimal.
> > > > >
> > > > > About renaming the others with `XXNames()`: well, to me it feels
> > still
> > > > not
> > > > > very worthy since although it is not a big burden, it seems also
> not
> > a
> > > > big
> > > > > "return" if we name the newly added function `topicSet()`.
> > > > >
> > > > >
> > > > > Guozhang
> > > > >
> > > > >
> > > > > On Fri, Jul 20, 2018 at 7:38 PM, Nishanth Pradeep <
> > > nishanth...@gmail.com
> > > > >
> > > > > wrote:
> > > > >
> > > > >> I definitely agree with you on deprecating topics().
> > > > >>
> > > > >> I also think changing the method names for consistency is
> > reasonable,
> > > > since
> > > > >> there is no functionality change. Although, I can be convinced
> > either
> > > > way
> > > > >> on this one.
> > > > >>
> > > > >> Best,
> > > > >> Nishanth Pradeep
> > > > >> On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax <
> > > matth...@confluent.io
> > > > >
> > > > >> wrote:
> > > > >>
> > > > >>> I would still deprecate existing `topics()` method. If users
> need a
> > > > >>> String, they can call `topicSet().toString()`.
> > > > >>>
> > > > >>> It's just a personal preference, because I believe it's good to
> > keep
> > > > the
> > > > >>> API "minimal".
> > > > >>>
> > > > >>> About renaming the other methods: I thinks it's a very small
> burden
> > > to
> > > > >>> deprecate the existing methods and add them with new names. Also
> > just
> > > > my
> > > > >>> 2 cents.
> > > > >>>
> > > > >>> Would be good to see what others think.
> > > > >>>
> > > > >>>
> > > > >>> -Matthias
> > > > >>>
> > > > >>> On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
> > > >  Understood, Guozhang.
> > > > 
> > > >  Thanks for the help, everyone! I have updated the KIP. Let me
> know
> > > if
> > > > >> you
> > > >  any other thoughts or suggestions.
> > > > 
> > > >  Best,
> > > >  Nishanth Pradeep
> > > > 
> > > >  On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang <
> wangg...@gmail.com
> > >
> > > > >>> wrote:
> > > > 
> > > > > I see.
> > > > >
> > > > > Well, I think if we add a new function like topicSet() it is
> less
> > > > >>> needed to
> > > > > deprecate topics() as it returns "{topic1, topic2, ..}" which
> is
> > > sort
> > > > >> of
> > > > > non-overlapping in usage with the new API.
> > > > >
> > > > >
> > > > > Guozhang
> > > > >
> > > > > On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep <
> > > > >>> nishanth...@gmail.com>
> > > > > wrote:
> > > > >
> > > > >> That is what I meant. I will add topicSet() instead of
> changing
> > > the
> > > > >> signature of topics() for compatibility reasons. But should we
> > not
> > > > >> add
> > > > >>> a
> > > > >> @deprecated flag for topics() or do you want to keep it around
> > for
> > > > >> the
> > > > > long
> > > > >> run?
> > > > >>
> > > > >> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang <
> > wangg...@gmail.com
> > > >
> > > > > wrote:
> > > > >>
> > > > >>> We cannot change the signature of the function named "topics"
> > > from
> > > > >> "String"
> > > > >>> to "Set", as Matthias mentioned it is a compatibility
> > > > >> breaking
> > > > >>> change.
> > > > >>>
> > > > >>> That's why I was proposing add a new function like
> "Set
> > > > >>> topicSet()", while keeping "String topics()" as is.
> > > > >>>
> > > > >>> Guozhang
> > > > >>>
> > > > >>> On Thu, Jul 19, 2018 at 5:22 PM, Nishanth 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

I have updated the KIP

.

Changes to the KIP:

   - Removed topics() from the Public Interface and Proposed Changes
   sections.
   - Added topics() to the Deprecation plan.

Thanks again for the feedback.

Best,
Nishanth Pradeep

On Tue, Jul 24, 2018 at 11:21 AM Guozhang Wang  wrote:

> We should not remove it immediately in the up coming 2.1 release. Usually
> we first mark an API as deprecated, and consider removing it only after it
> has been deprecated for at least one major release period.
>
>
> Guozhang
>
> On Mon, Jul 23, 2018 at 7:40 PM, Nishanth Pradeep 
> wrote:
>
> > Sounds good to me too.
> >
> > As far as deprecating goes -- should the topics() method removed
> completely
> > or should it have a @deprecated annotation for removal in some future
> > version?
> >
> > Best,
> > Nishanth Pradeep
> >
> > On Sun, Jul 22, 2018 at 1:32 PM Matthias J. Sax 
> > wrote:
> >
> > > Works for me.
> > >
> > > On 7/22/18 9:48 AM, Guozhang Wang wrote:
> > > > I think I can be convinced with deprecating topics() to keep API
> > minimal.
> > > >
> > > > About renaming the others with `XXNames()`: well, to me it feels
> still
> > > not
> > > > very worthy since although it is not a big burden, it seems also not
> a
> > > big
> > > > "return" if we name the newly added function `topicSet()`.
> > > >
> > > >
> > > > Guozhang
> > > >
> > > >
> > > > On Fri, Jul 20, 2018 at 7:38 PM, Nishanth Pradeep <
> > nishanth...@gmail.com
> > > >
> > > > wrote:
> > > >
> > > >> I definitely agree with you on deprecating topics().
> > > >>
> > > >> I also think changing the method names for consistency is
> reasonable,
> > > since
> > > >> there is no functionality change. Although, I can be convinced
> either
> > > way
> > > >> on this one.
> > > >>
> > > >> Best,
> > > >> Nishanth Pradeep
> > > >> On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax <
> > matth...@confluent.io
> > > >
> > > >> wrote:
> > > >>
> > > >>> I would still deprecate existing `topics()` method. If users need a
> > > >>> String, they can call `topicSet().toString()`.
> > > >>>
> > > >>> It's just a personal preference, because I believe it's good to
> keep
> > > the
> > > >>> API "minimal".
> > > >>>
> > > >>> About renaming the other methods: I thinks it's a very small burden
> > to
> > > >>> deprecate the existing methods and add them with new names. Also
> just
> > > my
> > > >>> 2 cents.
> > > >>>
> > > >>> Would be good to see what others think.
> > > >>>
> > > >>>
> > > >>> -Matthias
> > > >>>
> > > >>> On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
> > >  Understood, Guozhang.
> > > 
> > >  Thanks for the help, everyone! I have updated the KIP. Let me know
> > if
> > > >> you
> > >  any other thoughts or suggestions.
> > > 
> > >  Best,
> > >  Nishanth Pradeep
> > > 
> > >  On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang  >
> > > >>> wrote:
> > > 
> > > > I see.
> > > >
> > > > Well, I think if we add a new function like topicSet() it is less
> > > >>> needed to
> > > > deprecate topics() as it returns "{topic1, topic2, ..}" which is
> > sort
> > > >> of
> > > > non-overlapping in usage with the new API.
> > > >
> > > >
> > > > Guozhang
> > > >
> > > > On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep <
> > > >>> nishanth...@gmail.com>
> > > > wrote:
> > > >
> > > >> That is what I meant. I will add topicSet() instead of changing
> > the
> > > >> signature of topics() for compatibility reasons. But should we
> not
> > > >> add
> > > >>> a
> > > >> @deprecated flag for topics() or do you want to keep it around
> for
> > > >> the
> > > > long
> > > >> run?
> > > >>
> > > >> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang <
> wangg...@gmail.com
> > >
> > > > wrote:
> > > >>
> > > >>> We cannot change the signature of the function named "topics"
> > from
> > > >> "String"
> > > >>> to "Set", as Matthias mentioned it is a compatibility
> > > >> breaking
> > > >>> change.
> > > >>>
> > > >>> That's why I was proposing add a new function like "Set
> > > >>> topicSet()", while keeping "String topics()" as is.
> > > >>>
> > > >>> Guozhang
> > > >>>
> > > >>> On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
> > > > nishanth...@gmail.com
> > > >>>
> > > >>> wrote:
> > > >>>
> > >  Right, adding topicNames() instead of changing the return type
> > of
> > > >>> topics()
> > >  in order preserve backwards compatibility is a good idea. But
> is
> > > it
> > > > not
> > >  better to depreciate topics() because it would be redundant?
> In
> > > our
> > > >> case,
> > >  it would only be calling topicNames/topicSet#toString().
> > > 
> > >  I still agree that perhaps changing the other API's might be
> 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

We should not remove it immediately in the up coming 2.1 release. Usually
we first mark an API as deprecated, and consider removing it only after it
has been deprecated for at least one major release period.


Guozhang

On Mon, Jul 23, 2018 at 7:40 PM, Nishanth Pradeep 
wrote:

> Sounds good to me too.
>
> As far as deprecating goes -- should the topics() method removed completely
> or should it have a @deprecated annotation for removal in some future
> version?
>
> Best,
> Nishanth Pradeep
>
> On Sun, Jul 22, 2018 at 1:32 PM Matthias J. Sax 
> wrote:
>
> > Works for me.
> >
> > On 7/22/18 9:48 AM, Guozhang Wang wrote:
> > > I think I can be convinced with deprecating topics() to keep API
> minimal.
> > >
> > > About renaming the others with `XXNames()`: well, to me it feels still
> > not
> > > very worthy since although it is not a big burden, it seems also not a
> > big
> > > "return" if we name the newly added function `topicSet()`.
> > >
> > >
> > > Guozhang
> > >
> > >
> > > On Fri, Jul 20, 2018 at 7:38 PM, Nishanth Pradeep <
> nishanth...@gmail.com
> > >
> > > wrote:
> > >
> > >> I definitely agree with you on deprecating topics().
> > >>
> > >> I also think changing the method names for consistency is reasonable,
> > since
> > >> there is no functionality change. Although, I can be convinced either
> > way
> > >> on this one.
> > >>
> > >> Best,
> > >> Nishanth Pradeep
> > >> On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax <
> matth...@confluent.io
> > >
> > >> wrote:
> > >>
> > >>> I would still deprecate existing `topics()` method. If users need a
> > >>> String, they can call `topicSet().toString()`.
> > >>>
> > >>> It's just a personal preference, because I believe it's good to keep
> > the
> > >>> API "minimal".
> > >>>
> > >>> About renaming the other methods: I thinks it's a very small burden
> to
> > >>> deprecate the existing methods and add them with new names. Also just
> > my
> > >>> 2 cents.
> > >>>
> > >>> Would be good to see what others think.
> > >>>
> > >>>
> > >>> -Matthias
> > >>>
> > >>> On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
> >  Understood, Guozhang.
> > 
> >  Thanks for the help, everyone! I have updated the KIP. Let me know
> if
> > >> you
> >  any other thoughts or suggestions.
> > 
> >  Best,
> >  Nishanth Pradeep
> > 
> >  On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang 
> > >>> wrote:
> > 
> > > I see.
> > >
> > > Well, I think if we add a new function like topicSet() it is less
> > >>> needed to
> > > deprecate topics() as it returns "{topic1, topic2, ..}" which is
> sort
> > >> of
> > > non-overlapping in usage with the new API.
> > >
> > >
> > > Guozhang
> > >
> > > On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep <
> > >>> nishanth...@gmail.com>
> > > wrote:
> > >
> > >> That is what I meant. I will add topicSet() instead of changing
> the
> > >> signature of topics() for compatibility reasons. But should we not
> > >> add
> > >>> a
> > >> @deprecated flag for topics() or do you want to keep it around for
> > >> the
> > > long
> > >> run?
> > >>
> > >> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang  >
> > > wrote:
> > >>
> > >>> We cannot change the signature of the function named "topics"
> from
> > >> "String"
> > >>> to "Set", as Matthias mentioned it is a compatibility
> > >> breaking
> > >>> change.
> > >>>
> > >>> That's why I was proposing add a new function like "Set
> > >>> topicSet()", while keeping "String topics()" as is.
> > >>>
> > >>> Guozhang
> > >>>
> > >>> On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
> > > nishanth...@gmail.com
> > >>>
> > >>> wrote:
> > >>>
> >  Right, adding topicNames() instead of changing the return type
> of
> > >>> topics()
> >  in order preserve backwards compatibility is a good idea. But is
> > it
> > > not
> >  better to depreciate topics() because it would be redundant? In
> > our
> > >> case,
> >  it would only be calling topicNames/topicSet#toString().
> > 
> >  I still agree that perhaps changing the other API's might be
> > >> unnecessary
> >  since it's only a name change.
> > 
> >  I have made the change to the KIP to only add, not change,
> > > preexisting
> >  APIs. But where do we stand on deprecating topics()?
> > 
> >  Best,
> >  Nishanth Pradeep
> > 
> >  On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang <
> wangg...@gmail.com
> > >
> > >>> wrote:
> > 
> > > Personally I'd prefer to keep the deprecation-related changes
> as
> > >> small
> > >>> as
> > > possible unless they are really necessary, and hence I'd prefer
> > to
> > >> just
> >  add
> > >
> > > List topicList()  /* or Set topicSet() */
> > >
> > 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

Sounds good to me too.

As far as deprecating goes -- should the topics() method removed completely
or should it have a @deprecated annotation for removal in some future
version?

Best,
Nishanth Pradeep

On Sun, Jul 22, 2018 at 1:32 PM Matthias J. Sax 
wrote:

> Works for me.
>
> On 7/22/18 9:48 AM, Guozhang Wang wrote:
> > I think I can be convinced with deprecating topics() to keep API minimal.
> >
> > About renaming the others with `XXNames()`: well, to me it feels still
> not
> > very worthy since although it is not a big burden, it seems also not a
> big
> > "return" if we name the newly added function `topicSet()`.
> >
> >
> > Guozhang
> >
> >
> > On Fri, Jul 20, 2018 at 7:38 PM, Nishanth Pradeep  >
> > wrote:
> >
> >> I definitely agree with you on deprecating topics().
> >>
> >> I also think changing the method names for consistency is reasonable,
> since
> >> there is no functionality change. Although, I can be convinced either
> way
> >> on this one.
> >>
> >> Best,
> >> Nishanth Pradeep
> >> On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax  >
> >> wrote:
> >>
> >>> I would still deprecate existing `topics()` method. If users need a
> >>> String, they can call `topicSet().toString()`.
> >>>
> >>> It's just a personal preference, because I believe it's good to keep
> the
> >>> API "minimal".
> >>>
> >>> About renaming the other methods: I thinks it's a very small burden to
> >>> deprecate the existing methods and add them with new names. Also just
> my
> >>> 2 cents.
> >>>
> >>> Would be good to see what others think.
> >>>
> >>>
> >>> -Matthias
> >>>
> >>> On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
>  Understood, Guozhang.
> 
>  Thanks for the help, everyone! I have updated the KIP. Let me know if
> >> you
>  any other thoughts or suggestions.
> 
>  Best,
>  Nishanth Pradeep
> 
>  On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang 
> >>> wrote:
> 
> > I see.
> >
> > Well, I think if we add a new function like topicSet() it is less
> >>> needed to
> > deprecate topics() as it returns "{topic1, topic2, ..}" which is sort
> >> of
> > non-overlapping in usage with the new API.
> >
> >
> > Guozhang
> >
> > On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep <
> >>> nishanth...@gmail.com>
> > wrote:
> >
> >> That is what I meant. I will add topicSet() instead of changing the
> >> signature of topics() for compatibility reasons. But should we not
> >> add
> >>> a
> >> @deprecated flag for topics() or do you want to keep it around for
> >> the
> > long
> >> run?
> >>
> >> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang 
> > wrote:
> >>
> >>> We cannot change the signature of the function named "topics" from
> >> "String"
> >>> to "Set", as Matthias mentioned it is a compatibility
> >> breaking
> >>> change.
> >>>
> >>> That's why I was proposing add a new function like "Set
> >>> topicSet()", while keeping "String topics()" as is.
> >>>
> >>> Guozhang
> >>>
> >>> On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
> > nishanth...@gmail.com
> >>>
> >>> wrote:
> >>>
>  Right, adding topicNames() instead of changing the return type of
> >>> topics()
>  in order preserve backwards compatibility is a good idea. But is
> it
> > not
>  better to depreciate topics() because it would be redundant? In
> our
> >> case,
>  it would only be calling topicNames/topicSet#toString().
> 
>  I still agree that perhaps changing the other API's might be
> >> unnecessary
>  since it's only a name change.
> 
>  I have made the change to the KIP to only add, not change,
> > preexisting
>  APIs. But where do we stand on deprecating topics()?
> 
>  Best,
>  Nishanth Pradeep
> 
>  On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang  >
> >>> wrote:
> 
> > Personally I'd prefer to keep the deprecation-related changes as
> >> small
> >>> as
> > possible unless they are really necessary, and hence I'd prefer
> to
> >> just
>  add
> >
> > List topicList()  /* or Set topicSet() */
> >
> > in addition to topicPattern to Source, in addition to
>  `topicNameExtractor`
> > to Sink, and leaving the current APIs as-is.
> >
> > Guozhang
> >
> > On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
> >>> matth...@confluent.io
> >
> > wrote:
> >
> >> Thanks for updating the KIP.
> >>
> >> The current `Source` interface has a method `String topics()`
> > atm.
>  Thus,
> >> we cannot just add `Set Source#topics()` because this
> > would
> >> replace the existing method and would be an incompatible change.
> >>
> >> I 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

Works for me.

On 7/22/18 9:48 AM, Guozhang Wang wrote:
> I think I can be convinced with deprecating topics() to keep API minimal.
> 
> About renaming the others with `XXNames()`: well, to me it feels still not
> very worthy since although it is not a big burden, it seems also not a big
> "return" if we name the newly added function `topicSet()`.
> 
> 
> Guozhang
> 
> 
> On Fri, Jul 20, 2018 at 7:38 PM, Nishanth Pradeep 
> wrote:
> 
>> I definitely agree with you on deprecating topics().
>>
>> I also think changing the method names for consistency is reasonable, since
>> there is no functionality change. Although, I can be convinced either way
>> on this one.
>>
>> Best,
>> Nishanth Pradeep
>> On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax 
>> wrote:
>>
>>> I would still deprecate existing `topics()` method. If users need a
>>> String, they can call `topicSet().toString()`.
>>>
>>> It's just a personal preference, because I believe it's good to keep the
>>> API "minimal".
>>>
>>> About renaming the other methods: I thinks it's a very small burden to
>>> deprecate the existing methods and add them with new names. Also just my
>>> 2 cents.
>>>
>>> Would be good to see what others think.
>>>
>>>
>>> -Matthias
>>>
>>> On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
 Understood, Guozhang.

 Thanks for the help, everyone! I have updated the KIP. Let me know if
>> you
 any other thoughts or suggestions.

 Best,
 Nishanth Pradeep

 On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang 
>>> wrote:

> I see.
>
> Well, I think if we add a new function like topicSet() it is less
>>> needed to
> deprecate topics() as it returns "{topic1, topic2, ..}" which is sort
>> of
> non-overlapping in usage with the new API.
>
>
> Guozhang
>
> On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep <
>>> nishanth...@gmail.com>
> wrote:
>
>> That is what I meant. I will add topicSet() instead of changing the
>> signature of topics() for compatibility reasons. But should we not
>> add
>>> a
>> @deprecated flag for topics() or do you want to keep it around for
>> the
> long
>> run?
>>
>> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang 
> wrote:
>>
>>> We cannot change the signature of the function named "topics" from
>> "String"
>>> to "Set", as Matthias mentioned it is a compatibility
>> breaking
>>> change.
>>>
>>> That's why I was proposing add a new function like "Set
>>> topicSet()", while keeping "String topics()" as is.
>>>
>>> Guozhang
>>>
>>> On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
> nishanth...@gmail.com
>>>
>>> wrote:
>>>
 Right, adding topicNames() instead of changing the return type of
>>> topics()
 in order preserve backwards compatibility is a good idea. But is it
> not
 better to depreciate topics() because it would be redundant? In our
>> case,
 it would only be calling topicNames/topicSet#toString().

 I still agree that perhaps changing the other API's might be
>> unnecessary
 since it's only a name change.

 I have made the change to the KIP to only add, not change,
> preexisting
 APIs. But where do we stand on deprecating topics()?

 Best,
 Nishanth Pradeep

 On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang 
>>> wrote:

> Personally I'd prefer to keep the deprecation-related changes as
>> small
>>> as
> possible unless they are really necessary, and hence I'd prefer to
>> just
 add
>
> List topicList()  /* or Set topicSet() */
>
> in addition to topicPattern to Source, in addition to
 `topicNameExtractor`
> to Sink, and leaving the current APIs as-is.
>
> Guozhang
>
> On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
>>> matth...@confluent.io
>
> wrote:
>
>> Thanks for updating the KIP.
>>
>> The current `Source` interface has a method `String topics()`
> atm.
 Thus,
>> we cannot just add `Set Source#topics()` because this
> would
>> replace the existing method and would be an incompatible change.
>>
>> I think, we should deprecate `String topics()` and add a method
>> with
>> different name:
>>
>> `Set Source#topicNames()`
>>
>> The method name `topicNames` is more appropriate anyway, as we
>>> return a
>> set of String (ie, names) but no `Topic` objects. This raises one
>>> more
>> thought: we might want to rename `Processor#stores()` to
>> `Processor#storeNames()` as well ass `Sink#topic()` to
>> `Sink#topicName()`, too. This would keep the naming in the API
> consistent.
>>

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

I think I can be convinced with deprecating topics() to keep API minimal.

About renaming the others with `XXNames()`: well, to me it feels still not
very worthy since although it is not a big burden, it seems also not a big
"return" if we name the newly added function `topicSet()`.


Guozhang


On Fri, Jul 20, 2018 at 7:38 PM, Nishanth Pradeep 
wrote:

> I definitely agree with you on deprecating topics().
>
> I also think changing the method names for consistency is reasonable, since
> there is no functionality change. Although, I can be convinced either way
> on this one.
>
> Best,
> Nishanth Pradeep
> On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax 
> wrote:
>
> > I would still deprecate existing `topics()` method. If users need a
> > String, they can call `topicSet().toString()`.
> >
> > It's just a personal preference, because I believe it's good to keep the
> > API "minimal".
> >
> > About renaming the other methods: I thinks it's a very small burden to
> > deprecate the existing methods and add them with new names. Also just my
> > 2 cents.
> >
> > Would be good to see what others think.
> >
> >
> > -Matthias
> >
> > On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
> > > Understood, Guozhang.
> > >
> > > Thanks for the help, everyone! I have updated the KIP. Let me know if
> you
> > > any other thoughts or suggestions.
> > >
> > > Best,
> > > Nishanth Pradeep
> > >
> > > On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang 
> > wrote:
> > >
> > >> I see.
> > >>
> > >> Well, I think if we add a new function like topicSet() it is less
> > needed to
> > >> deprecate topics() as it returns "{topic1, topic2, ..}" which is sort
> of
> > >> non-overlapping in usage with the new API.
> > >>
> > >>
> > >> Guozhang
> > >>
> > >> On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep <
> > nishanth...@gmail.com>
> > >> wrote:
> > >>
> > >>> That is what I meant. I will add topicSet() instead of changing the
> > >>> signature of topics() for compatibility reasons. But should we not
> add
> > a
> > >>> @deprecated flag for topics() or do you want to keep it around for
> the
> > >> long
> > >>> run?
> > >>>
> > >>> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang 
> > >> wrote:
> > >>>
> >  We cannot change the signature of the function named "topics" from
> > >>> "String"
> >  to "Set", as Matthias mentioned it is a compatibility
> breaking
> >  change.
> > 
> >  That's why I was proposing add a new function like "Set
> >  topicSet()", while keeping "String topics()" as is.
> > 
> >  Guozhang
> > 
> >  On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
> > >> nishanth...@gmail.com
> > 
> >  wrote:
> > 
> > > Right, adding topicNames() instead of changing the return type of
> >  topics()
> > > in order preserve backwards compatibility is a good idea. But is it
> > >> not
> > > better to depreciate topics() because it would be redundant? In our
> > >>> case,
> > > it would only be calling topicNames/topicSet#toString().
> > >
> > > I still agree that perhaps changing the other API's might be
> > >>> unnecessary
> > > since it's only a name change.
> > >
> > > I have made the change to the KIP to only add, not change,
> > >> preexisting
> > > APIs. But where do we stand on deprecating topics()?
> > >
> > > Best,
> > > Nishanth Pradeep
> > >
> > > On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang 
> >  wrote:
> > >
> > >> Personally I'd prefer to keep the deprecation-related changes as
> > >>> small
> >  as
> > >> possible unless they are really necessary, and hence I'd prefer to
> > >>> just
> > > add
> > >>
> > >> List topicList()  /* or Set topicSet() */
> > >>
> > >> in addition to topicPattern to Source, in addition to
> > > `topicNameExtractor`
> > >> to Sink, and leaving the current APIs as-is.
> > >>
> > >> Guozhang
> > >>
> > >> On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
> >  matth...@confluent.io
> > >>
> > >> wrote:
> > >>
> > >>> Thanks for updating the KIP.
> > >>>
> > >>> The current `Source` interface has a method `String topics()`
> > >> atm.
> > > Thus,
> > >>> we cannot just add `Set Source#topics()` because this
> > >> would
> > >>> replace the existing method and would be an incompatible change.
> > >>>
> > >>> I think, we should deprecate `String topics()` and add a method
> > >>> with
> > >>> different name:
> > >>>
> > >>> `Set Source#topicNames()`
> > >>>
> > >>> The method name `topicNames` is more appropriate anyway, as we
> >  return a
> > >>> set of String (ie, names) but no `Topic` objects. This raises one
> >  more
> > >>> thought: we might want to rename `Processor#stores()` to
> > >>> `Processor#storeNames()` as well ass `Sink#topic()` to
> > >>> `Sink#topicName()`, too. This would keep the naming in the API
> > >> 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

I definitely agree with you on deprecating topics().

I also think changing the method names for consistency is reasonable, since
there is no functionality change. Although, I can be convinced either way
on this one.

Best,
Nishanth Pradeep
On Fri, Jul 20, 2018 at 12:15 PM Matthias J. Sax 
wrote:

> I would still deprecate existing `topics()` method. If users need a
> String, they can call `topicSet().toString()`.
>
> It's just a personal preference, because I believe it's good to keep the
> API "minimal".
>
> About renaming the other methods: I thinks it's a very small burden to
> deprecate the existing methods and add them with new names. Also just my
> 2 cents.
>
> Would be good to see what others think.
>
>
> -Matthias
>
> On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
> > Understood, Guozhang.
> >
> > Thanks for the help, everyone! I have updated the KIP. Let me know if you
> > any other thoughts or suggestions.
> >
> > Best,
> > Nishanth Pradeep
> >
> > On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang 
> wrote:
> >
> >> I see.
> >>
> >> Well, I think if we add a new function like topicSet() it is less
> needed to
> >> deprecate topics() as it returns "{topic1, topic2, ..}" which is sort of
> >> non-overlapping in usage with the new API.
> >>
> >>
> >> Guozhang
> >>
> >> On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep <
> nishanth...@gmail.com>
> >> wrote:
> >>
> >>> That is what I meant. I will add topicSet() instead of changing the
> >>> signature of topics() for compatibility reasons. But should we not add
> a
> >>> @deprecated flag for topics() or do you want to keep it around for the
> >> long
> >>> run?
> >>>
> >>> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang 
> >> wrote:
> >>>
>  We cannot change the signature of the function named "topics" from
> >>> "String"
>  to "Set", as Matthias mentioned it is a compatibility breaking
>  change.
> 
>  That's why I was proposing add a new function like "Set
>  topicSet()", while keeping "String topics()" as is.
> 
>  Guozhang
> 
>  On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
> >> nishanth...@gmail.com
> 
>  wrote:
> 
> > Right, adding topicNames() instead of changing the return type of
>  topics()
> > in order preserve backwards compatibility is a good idea. But is it
> >> not
> > better to depreciate topics() because it would be redundant? In our
> >>> case,
> > it would only be calling topicNames/topicSet#toString().
> >
> > I still agree that perhaps changing the other API's might be
> >>> unnecessary
> > since it's only a name change.
> >
> > I have made the change to the KIP to only add, not change,
> >> preexisting
> > APIs. But where do we stand on deprecating topics()?
> >
> > Best,
> > Nishanth Pradeep
> >
> > On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang 
>  wrote:
> >
> >> Personally I'd prefer to keep the deprecation-related changes as
> >>> small
>  as
> >> possible unless they are really necessary, and hence I'd prefer to
> >>> just
> > add
> >>
> >> List topicList()  /* or Set topicSet() */
> >>
> >> in addition to topicPattern to Source, in addition to
> > `topicNameExtractor`
> >> to Sink, and leaving the current APIs as-is.
> >>
> >> Guozhang
> >>
> >> On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
>  matth...@confluent.io
> >>
> >> wrote:
> >>
> >>> Thanks for updating the KIP.
> >>>
> >>> The current `Source` interface has a method `String topics()`
> >> atm.
> > Thus,
> >>> we cannot just add `Set Source#topics()` because this
> >> would
> >>> replace the existing method and would be an incompatible change.
> >>>
> >>> I think, we should deprecate `String topics()` and add a method
> >>> with
> >>> different name:
> >>>
> >>> `Set Source#topicNames()`
> >>>
> >>> The method name `topicNames` is more appropriate anyway, as we
>  return a
> >>> set of String (ie, names) but no `Topic` objects. This raises one
>  more
> >>> thought: we might want to rename `Processor#stores()` to
> >>> `Processor#storeNames()` as well ass `Sink#topic()` to
> >>> `Sink#topicName()`, too. This would keep the naming in the API
> >> consistent.
> >>>
> >>>
> >>> WDYT? Hope that other can chime in as well.
> >>>
> >>>
> >>> -Matthias
> >>>
> >>> On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
>  I have revised the KIP
>   > 321%3A+Update+
> >>> TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
>  Here is a summary of the changes.
> 
> 1. Changed return type from String to Set for
> >> Source#topics().
> 
> Set Source#topics()
> 
> 2. Added method in TopologyDescription#Source to return 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

I would still deprecate existing `topics()` method. If users need a
String, they can call `topicSet().toString()`.

It's just a personal preference, because I believe it's good to keep the
API "minimal".

About renaming the other methods: I thinks it's a very small burden to
deprecate the existing methods and add them with new names. Also just my
2 cents.

Would be good to see what others think.


-Matthias

On 7/19/18 6:20 PM, Nishanth Pradeep wrote:
> Understood, Guozhang.
> 
> Thanks for the help, everyone! I have updated the KIP. Let me know if you
> any other thoughts or suggestions.
> 
> Best,
> Nishanth Pradeep
> 
> On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang  wrote:
> 
>> I see.
>>
>> Well, I think if we add a new function like topicSet() it is less needed to
>> deprecate topics() as it returns "{topic1, topic2, ..}" which is sort of
>> non-overlapping in usage with the new API.
>>
>>
>> Guozhang
>>
>> On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep 
>> wrote:
>>
>>> That is what I meant. I will add topicSet() instead of changing the
>>> signature of topics() for compatibility reasons. But should we not add a
>>> @deprecated flag for topics() or do you want to keep it around for the
>> long
>>> run?
>>>
>>> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang 
>> wrote:
>>>
 We cannot change the signature of the function named "topics" from
>>> "String"
 to "Set", as Matthias mentioned it is a compatibility breaking
 change.

 That's why I was proposing add a new function like "Set
 topicSet()", while keeping "String topics()" as is.

 Guozhang

 On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
>> nishanth...@gmail.com

 wrote:

> Right, adding topicNames() instead of changing the return type of
 topics()
> in order preserve backwards compatibility is a good idea. But is it
>> not
> better to depreciate topics() because it would be redundant? In our
>>> case,
> it would only be calling topicNames/topicSet#toString().
>
> I still agree that perhaps changing the other API's might be
>>> unnecessary
> since it's only a name change.
>
> I have made the change to the KIP to only add, not change,
>> preexisting
> APIs. But where do we stand on deprecating topics()?
>
> Best,
> Nishanth Pradeep
>
> On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang 
 wrote:
>
>> Personally I'd prefer to keep the deprecation-related changes as
>>> small
 as
>> possible unless they are really necessary, and hence I'd prefer to
>>> just
> add
>>
>> List topicList()  /* or Set topicSet() */
>>
>> in addition to topicPattern to Source, in addition to
> `topicNameExtractor`
>> to Sink, and leaving the current APIs as-is.
>>
>> Guozhang
>>
>> On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
 matth...@confluent.io
>>
>> wrote:
>>
>>> Thanks for updating the KIP.
>>>
>>> The current `Source` interface has a method `String topics()`
>> atm.
> Thus,
>>> we cannot just add `Set Source#topics()` because this
>> would
>>> replace the existing method and would be an incompatible change.
>>>
>>> I think, we should deprecate `String topics()` and add a method
>>> with
>>> different name:
>>>
>>> `Set Source#topicNames()`
>>>
>>> The method name `topicNames` is more appropriate anyway, as we
 return a
>>> set of String (ie, names) but no `Topic` objects. This raises one
 more
>>> thought: we might want to rename `Processor#stores()` to
>>> `Processor#storeNames()` as well ass `Sink#topic()` to
>>> `Sink#topicName()`, too. This would keep the naming in the API
>> consistent.
>>>
>>>
>>> WDYT? Hope that other can chime in as well.
>>>
>>>
>>> -Matthias
>>>
>>> On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
 I have revised the KIP
  321%3A+Update+
>>> TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
 Here is a summary of the changes.

1. Changed return type from String to Set for
>> Source#topics().

Set Source#topics()

2. Added method in TopologyDescription#Source to return the
> Pattern
>>> used
by the Source node.

Pattern Source#topicPattern()

3. Changed return type of Sink#topicNameExtractor from
>> Class> extends
TopicNameExtractor> to just TopicNameExtractor.

TopicNameExtractor Sink#topicNameExtractor()

 Best,
 Nishanth Pradeep

 On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang <
>>> wangg...@gmail.com
>
>>> wrote:

> Hi Nishanth,
>
> Since even combining these two the scope is still relatively
>>> 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

Understood, Guozhang.

Thanks for the help, everyone! I have updated the KIP. Let me know if you
any other thoughts or suggestions.

Best,
Nishanth Pradeep

On Thu, Jul 19, 2018 at 7:33 PM Guozhang Wang  wrote:

> I see.
>
> Well, I think if we add a new function like topicSet() it is less needed to
> deprecate topics() as it returns "{topic1, topic2, ..}" which is sort of
> non-overlapping in usage with the new API.
>
>
> Guozhang
>
> On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep 
> wrote:
>
> > That is what I meant. I will add topicSet() instead of changing the
> > signature of topics() for compatibility reasons. But should we not add a
> > @deprecated flag for topics() or do you want to keep it around for the
> long
> > run?
> >
> > On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang 
> wrote:
> >
> > > We cannot change the signature of the function named "topics" from
> > "String"
> > > to "Set", as Matthias mentioned it is a compatibility breaking
> > > change.
> > >
> > > That's why I was proposing add a new function like "Set
> > > topicSet()", while keeping "String topics()" as is.
> > >
> > > Guozhang
> > >
> > > On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep <
> nishanth...@gmail.com
> > >
> > > wrote:
> > >
> > > > Right, adding topicNames() instead of changing the return type of
> > > topics()
> > > > in order preserve backwards compatibility is a good idea. But is it
> not
> > > > better to depreciate topics() because it would be redundant? In our
> > case,
> > > > it would only be calling topicNames/topicSet#toString().
> > > >
> > > > I still agree that perhaps changing the other API's might be
> > unnecessary
> > > > since it's only a name change.
> > > >
> > > > I have made the change to the KIP to only add, not change,
> preexisting
> > > > APIs. But where do we stand on deprecating topics()?
> > > >
> > > > Best,
> > > > Nishanth Pradeep
> > > >
> > > > On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang 
> > > wrote:
> > > >
> > > > > Personally I'd prefer to keep the deprecation-related changes as
> > small
> > > as
> > > > > possible unless they are really necessary, and hence I'd prefer to
> > just
> > > > add
> > > > >
> > > > > List topicList()  /* or Set topicSet() */
> > > > >
> > > > > in addition to topicPattern to Source, in addition to
> > > > `topicNameExtractor`
> > > > > to Sink, and leaving the current APIs as-is.
> > > > >
> > > > > Guozhang
> > > > >
> > > > > On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
> > > matth...@confluent.io
> > > > >
> > > > > wrote:
> > > > >
> > > > > > Thanks for updating the KIP.
> > > > > >
> > > > > > The current `Source` interface has a method `String topics()`
> atm.
> > > > Thus,
> > > > > > we cannot just add `Set Source#topics()` because this
> would
> > > > > > replace the existing method and would be an incompatible change.
> > > > > >
> > > > > > I think, we should deprecate `String topics()` and add a method
> > with
> > > > > > different name:
> > > > > >
> > > > > > `Set Source#topicNames()`
> > > > > >
> > > > > > The method name `topicNames` is more appropriate anyway, as we
> > > return a
> > > > > > set of String (ie, names) but no `Topic` objects. This raises one
> > > more
> > > > > > thought: we might want to rename `Processor#stores()` to
> > > > > > `Processor#storeNames()` as well ass `Sink#topic()` to
> > > > > > `Sink#topicName()`, too. This would keep the naming in the API
> > > > > consistent.
> > > > > >
> > > > > >
> > > > > > WDYT? Hope that other can chime in as well.
> > > > > >
> > > > > >
> > > > > > -Matthias
> > > > > >
> > > > > > On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
> > > > > > > I have revised the KIP
> > > > > > >  > > > 321%3A+Update+
> > > > > > TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
> > > > > > > Here is a summary of the changes.
> > > > > > >
> > > > > > >1. Changed return type from String to Set for
> > > > > Source#topics().
> > > > > > >
> > > > > > >Set Source#topics()
> > > > > > >
> > > > > > >2. Added method in TopologyDescription#Source to return the
> > > > Pattern
> > > > > > used
> > > > > > >by the Source node.
> > > > > > >
> > > > > > >Pattern Source#topicPattern()
> > > > > > >
> > > > > > >3. Changed return type of Sink#topicNameExtractor from
> Class > > > > extends
> > > > > > >TopicNameExtractor> to just TopicNameExtractor.
> > > > > > >
> > > > > > >TopicNameExtractor Sink#topicNameExtractor()
> > > > > > >
> > > > > > > Best,
> > > > > > > Nishanth Pradeep
> > > > > > >
> > > > > > > On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang <
> > wangg...@gmail.com
> > > >
> > > > > > wrote:
> > > > > > >
> > > > > > >> Hi Nishanth,
> > > > > > >>
> > > > > > >> Since even combining these two the scope is still relatively
> > small
> > > > I'd
> > > > > > >> suggest just do it in one KIP if you're willing to work on
> them.
> > > If
> > > > > you
> > > > > > do
> > > 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

I see.

Well, I think if we add a new function like topicSet() it is less needed to
deprecate topics() as it returns "{topic1, topic2, ..}" which is sort of
non-overlapping in usage with the new API.


Guozhang

On Thu, Jul 19, 2018 at 5:31 PM, Nishanth Pradeep 
wrote:

> That is what I meant. I will add topicSet() instead of changing the
> signature of topics() for compatibility reasons. But should we not add a
> @deprecated flag for topics() or do you want to keep it around for the long
> run?
>
> On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang  wrote:
>
> > We cannot change the signature of the function named "topics" from
> "String"
> > to "Set", as Matthias mentioned it is a compatibility breaking
> > change.
> >
> > That's why I was proposing add a new function like "Set
> > topicSet()", while keeping "String topics()" as is.
> >
> > Guozhang
> >
> > On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep  >
> > wrote:
> >
> > > Right, adding topicNames() instead of changing the return type of
> > topics()
> > > in order preserve backwards compatibility is a good idea. But is it not
> > > better to depreciate topics() because it would be redundant? In our
> case,
> > > it would only be calling topicNames/topicSet#toString().
> > >
> > > I still agree that perhaps changing the other API's might be
> unnecessary
> > > since it's only a name change.
> > >
> > > I have made the change to the KIP to only add, not change, preexisting
> > > APIs. But where do we stand on deprecating topics()?
> > >
> > > Best,
> > > Nishanth Pradeep
> > >
> > > On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang 
> > wrote:
> > >
> > > > Personally I'd prefer to keep the deprecation-related changes as
> small
> > as
> > > > possible unless they are really necessary, and hence I'd prefer to
> just
> > > add
> > > >
> > > > List topicList()  /* or Set topicSet() */
> > > >
> > > > in addition to topicPattern to Source, in addition to
> > > `topicNameExtractor`
> > > > to Sink, and leaving the current APIs as-is.
> > > >
> > > > Guozhang
> > > >
> > > > On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
> > matth...@confluent.io
> > > >
> > > > wrote:
> > > >
> > > > > Thanks for updating the KIP.
> > > > >
> > > > > The current `Source` interface has a method `String topics()` atm.
> > > Thus,
> > > > > we cannot just add `Set Source#topics()` because this would
> > > > > replace the existing method and would be an incompatible change.
> > > > >
> > > > > I think, we should deprecate `String topics()` and add a method
> with
> > > > > different name:
> > > > >
> > > > > `Set Source#topicNames()`
> > > > >
> > > > > The method name `topicNames` is more appropriate anyway, as we
> > return a
> > > > > set of String (ie, names) but no `Topic` objects. This raises one
> > more
> > > > > thought: we might want to rename `Processor#stores()` to
> > > > > `Processor#storeNames()` as well ass `Sink#topic()` to
> > > > > `Sink#topicName()`, too. This would keep the naming in the API
> > > > consistent.
> > > > >
> > > > >
> > > > > WDYT? Hope that other can chime in as well.
> > > > >
> > > > >
> > > > > -Matthias
> > > > >
> > > > > On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
> > > > > > I have revised the KIP
> > > > > >  > > 321%3A+Update+
> > > > > TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
> > > > > > Here is a summary of the changes.
> > > > > >
> > > > > >1. Changed return type from String to Set for
> > > > Source#topics().
> > > > > >
> > > > > >Set Source#topics()
> > > > > >
> > > > > >2. Added method in TopologyDescription#Source to return the
> > > Pattern
> > > > > used
> > > > > >by the Source node.
> > > > > >
> > > > > >Pattern Source#topicPattern()
> > > > > >
> > > > > >3. Changed return type of Sink#topicNameExtractor from Class > > > extends
> > > > > >TopicNameExtractor> to just TopicNameExtractor.
> > > > > >
> > > > > >TopicNameExtractor Sink#topicNameExtractor()
> > > > > >
> > > > > > Best,
> > > > > > Nishanth Pradeep
> > > > > >
> > > > > > On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang <
> wangg...@gmail.com
> > >
> > > > > wrote:
> > > > > >
> > > > > >> Hi Nishanth,
> > > > > >>
> > > > > >> Since even combining these two the scope is still relatively
> small
> > > I'd
> > > > > >> suggest just do it in one KIP if you're willing to work on them.
> > If
> > > > you
> > > > > do
> > > > > >> not then pleas feel free to create the JIRA for the second step
> so
> > > > that
> > > > > >> others can pick it up.
> > > > > >>
> > > > > >>
> > > > > >> Guozhang
> > > > > >>
> > > > > >> On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax <
> > > > matth...@confluent.io
> > > > > >
> > > > > >> wrote:
> > > > > >>
> > > > > >>> There is no general protocol. We can include the changes in the
> > > > current
> > > > > >>> KIP or do a second KIP.
> > > > > >>>
> > > > > >>> If you don't want to include the change in this 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

That is what I meant. I will add topicSet() instead of changing the
signature of topics() for compatibility reasons. But should we not add a
@deprecated flag for topics() or do you want to keep it around for the long
run?

On Thu, Jul 19, 2018 at 7:27 PM Guozhang Wang  wrote:

> We cannot change the signature of the function named "topics" from "String"
> to "Set", as Matthias mentioned it is a compatibility breaking
> change.
>
> That's why I was proposing add a new function like "Set
> topicSet()", while keeping "String topics()" as is.
>
> Guozhang
>
> On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep 
> wrote:
>
> > Right, adding topicNames() instead of changing the return type of
> topics()
> > in order preserve backwards compatibility is a good idea. But is it not
> > better to depreciate topics() because it would be redundant? In our case,
> > it would only be calling topicNames/topicSet#toString().
> >
> > I still agree that perhaps changing the other API's might be unnecessary
> > since it's only a name change.
> >
> > I have made the change to the KIP to only add, not change, preexisting
> > APIs. But where do we stand on deprecating topics()?
> >
> > Best,
> > Nishanth Pradeep
> >
> > On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang 
> wrote:
> >
> > > Personally I'd prefer to keep the deprecation-related changes as small
> as
> > > possible unless they are really necessary, and hence I'd prefer to just
> > add
> > >
> > > List topicList()  /* or Set topicSet() */
> > >
> > > in addition to topicPattern to Source, in addition to
> > `topicNameExtractor`
> > > to Sink, and leaving the current APIs as-is.
> > >
> > > Guozhang
> > >
> > > On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax <
> matth...@confluent.io
> > >
> > > wrote:
> > >
> > > > Thanks for updating the KIP.
> > > >
> > > > The current `Source` interface has a method `String topics()` atm.
> > Thus,
> > > > we cannot just add `Set Source#topics()` because this would
> > > > replace the existing method and would be an incompatible change.
> > > >
> > > > I think, we should deprecate `String topics()` and add a method with
> > > > different name:
> > > >
> > > > `Set Source#topicNames()`
> > > >
> > > > The method name `topicNames` is more appropriate anyway, as we
> return a
> > > > set of String (ie, names) but no `Topic` objects. This raises one
> more
> > > > thought: we might want to rename `Processor#stores()` to
> > > > `Processor#storeNames()` as well ass `Sink#topic()` to
> > > > `Sink#topicName()`, too. This would keep the naming in the API
> > > consistent.
> > > >
> > > >
> > > > WDYT? Hope that other can chime in as well.
> > > >
> > > >
> > > > -Matthias
> > > >
> > > > On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
> > > > > I have revised the KIP
> > > > >  > 321%3A+Update+
> > > > TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
> > > > > Here is a summary of the changes.
> > > > >
> > > > >1. Changed return type from String to Set for
> > > Source#topics().
> > > > >
> > > > >Set Source#topics()
> > > > >
> > > > >2. Added method in TopologyDescription#Source to return the
> > Pattern
> > > > used
> > > > >by the Source node.
> > > > >
> > > > >Pattern Source#topicPattern()
> > > > >
> > > > >3. Changed return type of Sink#topicNameExtractor from Class > > extends
> > > > >TopicNameExtractor> to just TopicNameExtractor.
> > > > >
> > > > >TopicNameExtractor Sink#topicNameExtractor()
> > > > >
> > > > > Best,
> > > > > Nishanth Pradeep
> > > > >
> > > > > On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang  >
> > > > wrote:
> > > > >
> > > > >> Hi Nishanth,
> > > > >>
> > > > >> Since even combining these two the scope is still relatively small
> > I'd
> > > > >> suggest just do it in one KIP if you're willing to work on them.
> If
> > > you
> > > > do
> > > > >> not then pleas feel free to create the JIRA for the second step so
> > > that
> > > > >> others can pick it up.
> > > > >>
> > > > >>
> > > > >> Guozhang
> > > > >>
> > > > >> On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax <
> > > matth...@confluent.io
> > > > >
> > > > >> wrote:
> > > > >>
> > > > >>> There is no general protocol. We can include the changes in the
> > > current
> > > > >>> KIP or do a second KIP.
> > > > >>>
> > > > >>> If you don't want to include the change in this KIP, please
> create
> > a
> > > > new
> > > > >>> JIRA to track the other changes. You can assign the JIRA to
> > yourself
> > > > and
> > > > >>> start a second KIP if you want. You can also "link" both JIRAs as
> > > > >>> related to each other.
> > > > >>>
> > > > >>>
> > > > >>> -Matthias
> > > > >>>
> > > > >>> On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
> > > >  Thank you for the comments! I agree with these changes.
> > > > 
> > > >  So is the general protocol to update the same KIP, or is to
> scrap
> > > the
> > > >  current KIP and create a new one since 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

We cannot change the signature of the function named "topics" from "String"
to "Set", as Matthias mentioned it is a compatibility breaking
change.

That's why I was proposing add a new function like "Set
topicSet()", while keeping "String topics()" as is.

Guozhang

On Thu, Jul 19, 2018 at 5:22 PM, Nishanth Pradeep 
wrote:

> Right, adding topicNames() instead of changing the return type of topics()
> in order preserve backwards compatibility is a good idea. But is it not
> better to depreciate topics() because it would be redundant? In our case,
> it would only be calling topicNames/topicSet#toString().
>
> I still agree that perhaps changing the other API's might be unnecessary
> since it's only a name change.
>
> I have made the change to the KIP to only add, not change, preexisting
> APIs. But where do we stand on deprecating topics()?
>
> Best,
> Nishanth Pradeep
>
> On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang  wrote:
>
> > Personally I'd prefer to keep the deprecation-related changes as small as
> > possible unless they are really necessary, and hence I'd prefer to just
> add
> >
> > List topicList()  /* or Set topicSet() */
> >
> > in addition to topicPattern to Source, in addition to
> `topicNameExtractor`
> > to Sink, and leaving the current APIs as-is.
> >
> > Guozhang
> >
> > On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax  >
> > wrote:
> >
> > > Thanks for updating the KIP.
> > >
> > > The current `Source` interface has a method `String topics()` atm.
> Thus,
> > > we cannot just add `Set Source#topics()` because this would
> > > replace the existing method and would be an incompatible change.
> > >
> > > I think, we should deprecate `String topics()` and add a method with
> > > different name:
> > >
> > > `Set Source#topicNames()`
> > >
> > > The method name `topicNames` is more appropriate anyway, as we return a
> > > set of String (ie, names) but no `Topic` objects. This raises one more
> > > thought: we might want to rename `Processor#stores()` to
> > > `Processor#storeNames()` as well ass `Sink#topic()` to
> > > `Sink#topicName()`, too. This would keep the naming in the API
> > consistent.
> > >
> > >
> > > WDYT? Hope that other can chime in as well.
> > >
> > >
> > > -Matthias
> > >
> > > On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
> > > > I have revised the KIP
> > > >  321%3A+Update+
> > > TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
> > > > Here is a summary of the changes.
> > > >
> > > >1. Changed return type from String to Set for
> > Source#topics().
> > > >
> > > >Set Source#topics()
> > > >
> > > >2. Added method in TopologyDescription#Source to return the
> Pattern
> > > used
> > > >by the Source node.
> > > >
> > > >Pattern Source#topicPattern()
> > > >
> > > >3. Changed return type of Sink#topicNameExtractor from Class > extends
> > > >TopicNameExtractor> to just TopicNameExtractor.
> > > >
> > > >TopicNameExtractor Sink#topicNameExtractor()
> > > >
> > > > Best,
> > > > Nishanth Pradeep
> > > >
> > > > On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang 
> > > wrote:
> > > >
> > > >> Hi Nishanth,
> > > >>
> > > >> Since even combining these two the scope is still relatively small
> I'd
> > > >> suggest just do it in one KIP if you're willing to work on them. If
> > you
> > > do
> > > >> not then pleas feel free to create the JIRA for the second step so
> > that
> > > >> others can pick it up.
> > > >>
> > > >>
> > > >> Guozhang
> > > >>
> > > >> On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax <
> > matth...@confluent.io
> > > >
> > > >> wrote:
> > > >>
> > > >>> There is no general protocol. We can include the changes in the
> > current
> > > >>> KIP or do a second KIP.
> > > >>>
> > > >>> If you don't want to include the change in this KIP, please create
> a
> > > new
> > > >>> JIRA to track the other changes. You can assign the JIRA to
> yourself
> > > and
> > > >>> start a second KIP if you want. You can also "link" both JIRAs as
> > > >>> related to each other.
> > > >>>
> > > >>>
> > > >>> -Matthias
> > > >>>
> > > >>> On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
> > >  Thank you for the comments! I agree with these changes.
> > > 
> > >  So is the general protocol to update the same KIP, or is to scrap
> > the
> > >  current KIP and create a new one since it's beyond the scope of
> the
> > >  original KIP? I am happy to do either.
> > > 
> > >  On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax <
> > matth...@confluent.io
> > > >
> > >  wrote:
> > > 
> > > > Sounds good to me.
> > > >
> > > > -Matthias
> > > >
> > > > On 7/4/18 10:53 AM, Guozhang Wang wrote:
> > > >> After looked through the current TopologyDescription I think I'd
> > > want
> > > >>> to
> > > >> combine the suggestions from John and Matthias on the API
> > proposal.
> > > >> The
> > > >> motivations is that we have two relatively 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

Right, adding topicNames() instead of changing the return type of topics()
in order preserve backwards compatibility is a good idea. But is it not
better to depreciate topics() because it would be redundant? In our case,
it would only be calling topicNames/topicSet#toString().

I still agree that perhaps changing the other API's might be unnecessary
since it's only a name change.

I have made the change to the KIP to only add, not change, preexisting
APIs. But where do we stand on deprecating topics()?

Best,
Nishanth Pradeep

On Thu, Jul 19, 2018 at 1:44 PM Guozhang Wang  wrote:

> Personally I'd prefer to keep the deprecation-related changes as small as
> possible unless they are really necessary, and hence I'd prefer to just add
>
> List topicList()  /* or Set topicSet() */
>
> in addition to topicPattern to Source, in addition to `topicNameExtractor`
> to Sink, and leaving the current APIs as-is.
>
> Guozhang
>
> On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax 
> wrote:
>
> > Thanks for updating the KIP.
> >
> > The current `Source` interface has a method `String topics()` atm. Thus,
> > we cannot just add `Set Source#topics()` because this would
> > replace the existing method and would be an incompatible change.
> >
> > I think, we should deprecate `String topics()` and add a method with
> > different name:
> >
> > `Set Source#topicNames()`
> >
> > The method name `topicNames` is more appropriate anyway, as we return a
> > set of String (ie, names) but no `Topic` objects. This raises one more
> > thought: we might want to rename `Processor#stores()` to
> > `Processor#storeNames()` as well ass `Sink#topic()` to
> > `Sink#topicName()`, too. This would keep the naming in the API
> consistent.
> >
> >
> > WDYT? Hope that other can chime in as well.
> >
> >
> > -Matthias
> >
> > On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
> > > I have revised the KIP
> > >  > TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
> > > Here is a summary of the changes.
> > >
> > >1. Changed return type from String to Set for
> Source#topics().
> > >
> > >Set Source#topics()
> > >
> > >2. Added method in TopologyDescription#Source to return the Pattern
> > used
> > >by the Source node.
> > >
> > >Pattern Source#topicPattern()
> > >
> > >3. Changed return type of Sink#topicNameExtractor from Class extends
> > >TopicNameExtractor> to just TopicNameExtractor.
> > >
> > >TopicNameExtractor Sink#topicNameExtractor()
> > >
> > > Best,
> > > Nishanth Pradeep
> > >
> > > On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang 
> > wrote:
> > >
> > >> Hi Nishanth,
> > >>
> > >> Since even combining these two the scope is still relatively small I'd
> > >> suggest just do it in one KIP if you're willing to work on them. If
> you
> > do
> > >> not then pleas feel free to create the JIRA for the second step so
> that
> > >> others can pick it up.
> > >>
> > >>
> > >> Guozhang
> > >>
> > >> On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax <
> matth...@confluent.io
> > >
> > >> wrote:
> > >>
> > >>> There is no general protocol. We can include the changes in the
> current
> > >>> KIP or do a second KIP.
> > >>>
> > >>> If you don't want to include the change in this KIP, please create a
> > new
> > >>> JIRA to track the other changes. You can assign the JIRA to yourself
> > and
> > >>> start a second KIP if you want. You can also "link" both JIRAs as
> > >>> related to each other.
> > >>>
> > >>>
> > >>> -Matthias
> > >>>
> > >>> On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
> >  Thank you for the comments! I agree with these changes.
> > 
> >  So is the general protocol to update the same KIP, or is to scrap
> the
> >  current KIP and create a new one since it's beyond the scope of the
> >  original KIP? I am happy to do either.
> > 
> >  On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax <
> matth...@confluent.io
> > >
> >  wrote:
> > 
> > > Sounds good to me.
> > >
> > > -Matthias
> > >
> > > On 7/4/18 10:53 AM, Guozhang Wang wrote:
> > >> After looked through the current TopologyDescription I think I'd
> > want
> > >>> to
> > >> combine the suggestions from John and Matthias on the API
> proposal.
> > >> The
> > >> motivations is that we have two relatively different
> functionalities
> > >> provided from the APIs today:
> > >>
> > >> 1. Each interface's public functions, like
> > >> SourceNode#topics(), GlobalStore#source(), which returns
> non-String
> > >>> typed
> > >> data. The hope was to let users programmatically leverage on those
> > >> APIs
> > > for
> > >> runtime checking.
> > >> 2. Each interface's impl class also have an implicit toString()
> > > overridden
> > >> to print the necessary information. This was designed for
> debugging
> > >> purposes only during development cycles.
> > >>
> > >> What 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

Personally I'd prefer to keep the deprecation-related changes as small as
possible unless they are really necessary, and hence I'd prefer to just add

List topicList()  /* or Set topicSet() */

in addition to topicPattern to Source, in addition to `topicNameExtractor`
to Sink, and leaving the current APIs as-is.

Guozhang

On Thu, Jul 19, 2018 at 10:36 AM, Matthias J. Sax 
wrote:

> Thanks for updating the KIP.
>
> The current `Source` interface has a method `String topics()` atm. Thus,
> we cannot just add `Set Source#topics()` because this would
> replace the existing method and would be an incompatible change.
>
> I think, we should deprecate `String topics()` and add a method with
> different name:
>
> `Set Source#topicNames()`
>
> The method name `topicNames` is more appropriate anyway, as we return a
> set of String (ie, names) but no `Topic` objects. This raises one more
> thought: we might want to rename `Processor#stores()` to
> `Processor#storeNames()` as well ass `Sink#topic()` to
> `Sink#topicName()`, too. This would keep the naming in the API consistent.
>
>
> WDYT? Hope that other can chime in as well.
>
>
> -Matthias
>
> On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
> > I have revised the KIP
> >  TopologyDescription+to+better+represent+Source+and+Sink+Nodes>.
> > Here is a summary of the changes.
> >
> >1. Changed return type from String to Set for Source#topics().
> >
> >Set Source#topics()
> >
> >2. Added method in TopologyDescription#Source to return the Pattern
> used
> >by the Source node.
> >
> >Pattern Source#topicPattern()
> >
> >3. Changed return type of Sink#topicNameExtractor from Class >TopicNameExtractor> to just TopicNameExtractor.
> >
> >TopicNameExtractor Sink#topicNameExtractor()
> >
> > Best,
> > Nishanth Pradeep
> >
> > On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang 
> wrote:
> >
> >> Hi Nishanth,
> >>
> >> Since even combining these two the scope is still relatively small I'd
> >> suggest just do it in one KIP if you're willing to work on them. If you
> do
> >> not then pleas feel free to create the JIRA for the second step so that
> >> others can pick it up.
> >>
> >>
> >> Guozhang
> >>
> >> On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax  >
> >> wrote:
> >>
> >>> There is no general protocol. We can include the changes in the current
> >>> KIP or do a second KIP.
> >>>
> >>> If you don't want to include the change in this KIP, please create a
> new
> >>> JIRA to track the other changes. You can assign the JIRA to yourself
> and
> >>> start a second KIP if you want. You can also "link" both JIRAs as
> >>> related to each other.
> >>>
> >>>
> >>> -Matthias
> >>>
> >>> On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
>  Thank you for the comments! I agree with these changes.
> 
>  So is the general protocol to update the same KIP, or is to scrap the
>  current KIP and create a new one since it's beyond the scope of the
>  original KIP? I am happy to do either.
> 
>  On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax  >
>  wrote:
> 
> > Sounds good to me.
> >
> > -Matthias
> >
> > On 7/4/18 10:53 AM, Guozhang Wang wrote:
> >> After looked through the current TopologyDescription I think I'd
> want
> >>> to
> >> combine the suggestions from John and Matthias on the API proposal.
> >> The
> >> motivations is that we have two relatively different functionalities
> >> provided from the APIs today:
> >>
> >> 1. Each interface's public functions, like
> >> SourceNode#topics(), GlobalStore#source(), which returns non-String
> >>> typed
> >> data. The hope was to let users programmatically leverage on those
> >> APIs
> > for
> >> runtime checking.
> >> 2. Each interface's impl class also have an implicit toString()
> > overridden
> >> to print the necessary information. This was designed for debugging
> >> purposes only during development cycles.
> >>
> >> What we've observed so far, though, is that users leverage 2) much
> >> more
> >> than 1) in practice, since it is more convienent to parse strings
> >> than
> >> recursively calling the APIs to get non-string fields. On the other
> >>> hand,
> >> the discussion controversy is more around 1), not 2). As for 2)
> >> people
> > seem
> >> to be on the right page anyways: print the topic lists if it is not
> >> dynamic, or print extractor string format otherwise. For 1) above we
> > should
> >> probably have all three `Set topics()`, `Pattern
> >>> topicPattern()`
> >> and `TopicNameExtractor topicExtractor()`; while for 2) I feel
> > comfortable
> >> relying on the TopicNameExtractor#toString() in `Source#toString()`
> >>> impl
> >> since even if users do not override this function, the default value
> >> `className@hashcode` still looks fine to me.
> >>
> >>
> 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

Thanks for updating the KIP.

The current `Source` interface has a method `String topics()` atm. Thus,
we cannot just add `Set Source#topics()` because this would
replace the existing method and would be an incompatible change.

I think, we should deprecate `String topics()` and add a method with
different name:

`Set Source#topicNames()`

The method name `topicNames` is more appropriate anyway, as we return a
set of String (ie, names) but no `Topic` objects. This raises one more
thought: we might want to rename `Processor#stores()` to
`Processor#storeNames()` as well ass `Sink#topic()` to
`Sink#topicName()`, too. This would keep the naming in the API consistent.


WDYT? Hope that other can chime in as well.


-Matthias

On 7/18/18 7:49 PM, Nishanth Pradeep wrote:
> I have revised the KIP
> .
> Here is a summary of the changes.
> 
>1. Changed return type from String to Set for Source#topics().
> 
>Set Source#topics()
> 
>2. Added method in TopologyDescription#Source to return the Pattern used
>by the Source node.
> 
>Pattern Source#topicPattern()
> 
>3. Changed return type of Sink#topicNameExtractor from ClassTopicNameExtractor> to just TopicNameExtractor.
> 
>TopicNameExtractor Sink#topicNameExtractor()
> 
> Best,
> Nishanth Pradeep
> 
> On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang  wrote:
> 
>> Hi Nishanth,
>>
>> Since even combining these two the scope is still relatively small I'd
>> suggest just do it in one KIP if you're willing to work on them. If you do
>> not then pleas feel free to create the JIRA for the second step so that
>> others can pick it up.
>>
>>
>> Guozhang
>>
>> On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax 
>> wrote:
>>
>>> There is no general protocol. We can include the changes in the current
>>> KIP or do a second KIP.
>>>
>>> If you don't want to include the change in this KIP, please create a new
>>> JIRA to track the other changes. You can assign the JIRA to yourself and
>>> start a second KIP if you want. You can also "link" both JIRAs as
>>> related to each other.
>>>
>>>
>>> -Matthias
>>>
>>> On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
 Thank you for the comments! I agree with these changes.

 So is the general protocol to update the same KIP, or is to scrap the
 current KIP and create a new one since it's beyond the scope of the
 original KIP? I am happy to do either.

 On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax 
 wrote:

> Sounds good to me.
>
> -Matthias
>
> On 7/4/18 10:53 AM, Guozhang Wang wrote:
>> After looked through the current TopologyDescription I think I'd want
>>> to
>> combine the suggestions from John and Matthias on the API proposal.
>> The
>> motivations is that we have two relatively different functionalities
>> provided from the APIs today:
>>
>> 1. Each interface's public functions, like
>> SourceNode#topics(), GlobalStore#source(), which returns non-String
>>> typed
>> data. The hope was to let users programmatically leverage on those
>> APIs
> for
>> runtime checking.
>> 2. Each interface's impl class also have an implicit toString()
> overridden
>> to print the necessary information. This was designed for debugging
>> purposes only during development cycles.
>>
>> What we've observed so far, though, is that users leverage 2) much
>> more
>> than 1) in practice, since it is more convienent to parse strings
>> than
>> recursively calling the APIs to get non-string fields. On the other
>>> hand,
>> the discussion controversy is more around 1), not 2). As for 2)
>> people
> seem
>> to be on the right page anyways: print the topic lists if it is not
>> dynamic, or print extractor string format otherwise. For 1) above we
> should
>> probably have all three `Set topics()`, `Pattern
>>> topicPattern()`
>> and `TopicNameExtractor topicExtractor()`; while for 2) I feel
> comfortable
>> relying on the TopicNameExtractor#toString() in `Source#toString()`
>>> impl
>> since even if users do not override this function, the default value
>> `className@hashcode` still looks fine to me.
>>
>>
>> Guozhang
>>
>>
>>
>> On Tue, Jul 3, 2018 at 11:22 PM, Matthias J. Sax <
>>> matth...@confluent.io>
>> wrote:
>>
>>> I just double checked the discussion thread of KIP-120 that
>> introduced
>>> `TopologyDescription`. Back than the argument was, that using the
>>> simplest option might be sufficient because the description is
>> mostly
>>> used for debugging.
>>>
>>> Not sure if this argument holds. It seem that people built first
>> more
>>> sophisticated tools using TopologyDescription.
>>>
>>> Final note: if we really want to add `topicPattern()` we might want
>> to

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

I have revised the KIP
.
Here is a summary of the changes.

   1. Changed return type from String to Set for Source#topics().

   Set Source#topics()

   2. Added method in TopologyDescription#Source to return the Pattern used
   by the Source node.

   Pattern Source#topicPattern()

   3. Changed return type of Sink#topicNameExtractor from Class to just TopicNameExtractor.

   TopicNameExtractor Sink#topicNameExtractor()

Best,
Nishanth Pradeep

On Sun, Jul 15, 2018 at 11:24 PM Guozhang Wang  wrote:

> Hi Nishanth,
>
> Since even combining these two the scope is still relatively small I'd
> suggest just do it in one KIP if you're willing to work on them. If you do
> not then pleas feel free to create the JIRA for the second step so that
> others can pick it up.
>
>
> Guozhang
>
> On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax 
> wrote:
>
> > There is no general protocol. We can include the changes in the current
> > KIP or do a second KIP.
> >
> > If you don't want to include the change in this KIP, please create a new
> > JIRA to track the other changes. You can assign the JIRA to yourself and
> > start a second KIP if you want. You can also "link" both JIRAs as
> > related to each other.
> >
> >
> > -Matthias
> >
> > On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
> > > Thank you for the comments! I agree with these changes.
> > >
> > > So is the general protocol to update the same KIP, or is to scrap the
> > > current KIP and create a new one since it's beyond the scope of the
> > > original KIP? I am happy to do either.
> > >
> > > On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax 
> > > wrote:
> > >
> > >> Sounds good to me.
> > >>
> > >> -Matthias
> > >>
> > >> On 7/4/18 10:53 AM, Guozhang Wang wrote:
> > >>> After looked through the current TopologyDescription I think I'd want
> > to
> > >>> combine the suggestions from John and Matthias on the API proposal.
> The
> > >>> motivations is that we have two relatively different functionalities
> > >>> provided from the APIs today:
> > >>>
> > >>> 1. Each interface's public functions, like
> > >>> SourceNode#topics(), GlobalStore#source(), which returns non-String
> > typed
> > >>> data. The hope was to let users programmatically leverage on those
> APIs
> > >> for
> > >>> runtime checking.
> > >>> 2. Each interface's impl class also have an implicit toString()
> > >> overridden
> > >>> to print the necessary information. This was designed for debugging
> > >>> purposes only during development cycles.
> > >>>
> > >>> What we've observed so far, though, is that users leverage 2) much
> more
> > >>> than 1) in practice, since it is more convienent to parse strings
> than
> > >>> recursively calling the APIs to get non-string fields. On the other
> > hand,
> > >>> the discussion controversy is more around 1), not 2). As for 2)
> people
> > >> seem
> > >>> to be on the right page anyways: print the topic lists if it is not
> > >>> dynamic, or print extractor string format otherwise. For 1) above we
> > >> should
> > >>> probably have all three `Set topics()`, `Pattern
> > topicPattern()`
> > >>> and `TopicNameExtractor topicExtractor()`; while for 2) I feel
> > >> comfortable
> > >>> relying on the TopicNameExtractor#toString() in `Source#toString()`
> > impl
> > >>> since even if users do not override this function, the default value
> > >>> `className@hashcode` still looks fine to me.
> > >>>
> > >>>
> > >>> Guozhang
> > >>>
> > >>>
> > >>>
> > >>> On Tue, Jul 3, 2018 at 11:22 PM, Matthias J. Sax <
> > matth...@confluent.io>
> > >>> wrote:
> > >>>
> >  I just double checked the discussion thread of KIP-120 that
> introduced
> >  `TopologyDescription`. Back than the argument was, that using the
> >  simplest option might be sufficient because the description is
> mostly
> >  used for debugging.
> > 
> >  Not sure if this argument holds. It seem that people built first
> more
> >  sophisticated tools using TopologyDescription.
> > 
> >  Final note: if we really want to add `topicPattern()` we might want
> to
> >  deprecate `topic()` and replace with `Set topics()`,
> because a
> >  source node can take a multiple topics, too.
> > 
> >  Just adding this for completeness of context to the discussion.
> > 
> > 
> >  -Matthias
> > 
> >  On 7/3/18 11:09 PM, Matthias J. Sax wrote:
> > > John,
> > >
> > > I am a little bit on the fence. In retrospective, it might have
> been
> > > better to add `topic()` and `topicPattern()` to source node and
> > return
> > >> a
> > > proper `Pattern` object instead of the pattern as a String.
> > >
> > > All other "payload" is just names and thus String naturally. From
> my
> > > point of view `TopologyDescription` should represent the `Topology`
> > in
> > >> a
> > > "machine readable" form plus 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

Hi Nishanth,

Since even combining these two the scope is still relatively small I'd
suggest just do it in one KIP if you're willing to work on them. If you do
not then pleas feel free to create the JIRA for the second step so that
others can pick it up.


Guozhang

On Sun, Jul 15, 2018 at 6:14 PM, Matthias J. Sax 
wrote:

> There is no general protocol. We can include the changes in the current
> KIP or do a second KIP.
>
> If you don't want to include the change in this KIP, please create a new
> JIRA to track the other changes. You can assign the JIRA to yourself and
> start a second KIP if you want. You can also "link" both JIRAs as
> related to each other.
>
>
> -Matthias
>
> On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
> > Thank you for the comments! I agree with these changes.
> >
> > So is the general protocol to update the same KIP, or is to scrap the
> > current KIP and create a new one since it's beyond the scope of the
> > original KIP? I am happy to do either.
> >
> > On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax 
> > wrote:
> >
> >> Sounds good to me.
> >>
> >> -Matthias
> >>
> >> On 7/4/18 10:53 AM, Guozhang Wang wrote:
> >>> After looked through the current TopologyDescription I think I'd want
> to
> >>> combine the suggestions from John and Matthias on the API proposal. The
> >>> motivations is that we have two relatively different functionalities
> >>> provided from the APIs today:
> >>>
> >>> 1. Each interface's public functions, like
> >>> SourceNode#topics(), GlobalStore#source(), which returns non-String
> typed
> >>> data. The hope was to let users programmatically leverage on those APIs
> >> for
> >>> runtime checking.
> >>> 2. Each interface's impl class also have an implicit toString()
> >> overridden
> >>> to print the necessary information. This was designed for debugging
> >>> purposes only during development cycles.
> >>>
> >>> What we've observed so far, though, is that users leverage 2) much more
> >>> than 1) in practice, since it is more convienent to parse strings than
> >>> recursively calling the APIs to get non-string fields. On the other
> hand,
> >>> the discussion controversy is more around 1), not 2). As for 2) people
> >> seem
> >>> to be on the right page anyways: print the topic lists if it is not
> >>> dynamic, or print extractor string format otherwise. For 1) above we
> >> should
> >>> probably have all three `Set topics()`, `Pattern
> topicPattern()`
> >>> and `TopicNameExtractor topicExtractor()`; while for 2) I feel
> >> comfortable
> >>> relying on the TopicNameExtractor#toString() in `Source#toString()`
> impl
> >>> since even if users do not override this function, the default value
> >>> `className@hashcode` still looks fine to me.
> >>>
> >>>
> >>> Guozhang
> >>>
> >>>
> >>>
> >>> On Tue, Jul 3, 2018 at 11:22 PM, Matthias J. Sax <
> matth...@confluent.io>
> >>> wrote:
> >>>
>  I just double checked the discussion thread of KIP-120 that introduced
>  `TopologyDescription`. Back than the argument was, that using the
>  simplest option might be sufficient because the description is mostly
>  used for debugging.
> 
>  Not sure if this argument holds. It seem that people built first more
>  sophisticated tools using TopologyDescription.
> 
>  Final note: if we really want to add `topicPattern()` we might want to
>  deprecate `topic()` and replace with `Set topics()`, because a
>  source node can take a multiple topics, too.
> 
>  Just adding this for completeness of context to the discussion.
> 
> 
>  -Matthias
> 
>  On 7/3/18 11:09 PM, Matthias J. Sax wrote:
> > John,
> >
> > I am a little bit on the fence. In retrospective, it might have been
> > better to add `topic()` and `topicPattern()` to source node and
> return
> >> a
> > proper `Pattern` object instead of the pattern as a String.
> >
> > All other "payload" is just names and thus String naturally. From my
> > point of view `TopologyDescription` should represent the `Topology`
> in
> >> a
> > "machine readable" form plus a default "human readable" from via
> > `toString()` -- this does not imply that all return types should be
>  String.
> >
> > Let me know what you think. If you agree, we could even add
> > `Source#topicPattern()` in another KIP.
> >
> >
> > -Matthias
> >
> > On 6/26/18 3:45 PM, John Roesler wrote:
> >> Sorry for the late comment,
> >>
> >> Looking at the other pieces of TopologyDescription, I noticed that
>  pretty
> >> much all of the "payload" of these description nodes are strings.
>  Should we
> >> consider returning a string from `topicNameExtractor()` instead?
> >>
> >> In fact, if we did that, we could consider calling `toString()` on
> the
> >> extractor instead of returning the class name. This would allow
> >> authors
>  of
> >> the extractors to provide more information 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

There is no general protocol. We can include the changes in the current
KIP or do a second KIP.

If you don't want to include the change in this KIP, please create a new
JIRA to track the other changes. You can assign the JIRA to yourself and
start a second KIP if you want. You can also "link" both JIRAs as
related to each other.


-Matthias

On 7/15/18 12:50 PM, Nishanth Pradeep wrote:
> Thank you for the comments! I agree with these changes.
> 
> So is the general protocol to update the same KIP, or is to scrap the
> current KIP and create a new one since it's beyond the scope of the
> original KIP? I am happy to do either.
> 
> On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax 
> wrote:
> 
>> Sounds good to me.
>>
>> -Matthias
>>
>> On 7/4/18 10:53 AM, Guozhang Wang wrote:
>>> After looked through the current TopologyDescription I think I'd want to
>>> combine the suggestions from John and Matthias on the API proposal. The
>>> motivations is that we have two relatively different functionalities
>>> provided from the APIs today:
>>>
>>> 1. Each interface's public functions, like
>>> SourceNode#topics(), GlobalStore#source(), which returns non-String typed
>>> data. The hope was to let users programmatically leverage on those APIs
>> for
>>> runtime checking.
>>> 2. Each interface's impl class also have an implicit toString()
>> overridden
>>> to print the necessary information. This was designed for debugging
>>> purposes only during development cycles.
>>>
>>> What we've observed so far, though, is that users leverage 2) much more
>>> than 1) in practice, since it is more convienent to parse strings than
>>> recursively calling the APIs to get non-string fields. On the other hand,
>>> the discussion controversy is more around 1), not 2). As for 2) people
>> seem
>>> to be on the right page anyways: print the topic lists if it is not
>>> dynamic, or print extractor string format otherwise. For 1) above we
>> should
>>> probably have all three `Set topics()`, `Pattern topicPattern()`
>>> and `TopicNameExtractor topicExtractor()`; while for 2) I feel
>> comfortable
>>> relying on the TopicNameExtractor#toString() in `Source#toString()` impl
>>> since even if users do not override this function, the default value
>>> `className@hashcode` still looks fine to me.
>>>
>>>
>>> Guozhang
>>>
>>>
>>>
>>> On Tue, Jul 3, 2018 at 11:22 PM, Matthias J. Sax 
>>> wrote:
>>>
 I just double checked the discussion thread of KIP-120 that introduced
 `TopologyDescription`. Back than the argument was, that using the
 simplest option might be sufficient because the description is mostly
 used for debugging.

 Not sure if this argument holds. It seem that people built first more
 sophisticated tools using TopologyDescription.

 Final note: if we really want to add `topicPattern()` we might want to
 deprecate `topic()` and replace with `Set topics()`, because a
 source node can take a multiple topics, too.

 Just adding this for completeness of context to the discussion.


 -Matthias

 On 7/3/18 11:09 PM, Matthias J. Sax wrote:
> John,
>
> I am a little bit on the fence. In retrospective, it might have been
> better to add `topic()` and `topicPattern()` to source node and return
>> a
> proper `Pattern` object instead of the pattern as a String.
>
> All other "payload" is just names and thus String naturally. From my
> point of view `TopologyDescription` should represent the `Topology` in
>> a
> "machine readable" form plus a default "human readable" from via
> `toString()` -- this does not imply that all return types should be
 String.
>
> Let me know what you think. If you agree, we could even add
> `Source#topicPattern()` in another KIP.
>
>
> -Matthias
>
> On 6/26/18 3:45 PM, John Roesler wrote:
>> Sorry for the late comment,
>>
>> Looking at the other pieces of TopologyDescription, I noticed that
 pretty
>> much all of the "payload" of these description nodes are strings.
 Should we
>> consider returning a string from `topicNameExtractor()` instead?
>>
>> In fact, if we did that, we could consider calling `toString()` on the
>> extractor instead of returning the class name. This would allow
>> authors
 of
>> the extractors to provide more information about the extractor than
>> just
>> its name. This might be especially useful in the case of anonymous
>> implementations.
>>
>> Thanks for the KIP,
>> -John
>>
>> On Mon, Jun 25, 2018 at 11:52 PM Ted Yu  wrote:
>>
>>> My previous response was talking about the new method in
>>> InternalTopologyBuilder.
>>> The exception just means there is no uniform extractor for all the
 sinks.
>>>
>>> On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax <
 matth...@confluent.io>
>>> wrote:
>>>
 Ted,

 Why? Each 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Nishanth Pradeep]

Thank you for the comments! I agree with these changes.

So is the general protocol to update the same KIP, or is to scrap the
current KIP and create a new one since it's beyond the scope of the
original KIP? I am happy to do either.

On Wed, Jul 4, 2018 at 1:48 PM Matthias J. Sax 
wrote:

> Sounds good to me.
>
> -Matthias
>
> On 7/4/18 10:53 AM, Guozhang Wang wrote:
> > After looked through the current TopologyDescription I think I'd want to
> > combine the suggestions from John and Matthias on the API proposal. The
> > motivations is that we have two relatively different functionalities
> > provided from the APIs today:
> >
> > 1. Each interface's public functions, like
> > SourceNode#topics(), GlobalStore#source(), which returns non-String typed
> > data. The hope was to let users programmatically leverage on those APIs
> for
> > runtime checking.
> > 2. Each interface's impl class also have an implicit toString()
> overridden
> > to print the necessary information. This was designed for debugging
> > purposes only during development cycles.
> >
> > What we've observed so far, though, is that users leverage 2) much more
> > than 1) in practice, since it is more convienent to parse strings than
> > recursively calling the APIs to get non-string fields. On the other hand,
> > the discussion controversy is more around 1), not 2). As for 2) people
> seem
> > to be on the right page anyways: print the topic lists if it is not
> > dynamic, or print extractor string format otherwise. For 1) above we
> should
> > probably have all three `Set topics()`, `Pattern topicPattern()`
> > and `TopicNameExtractor topicExtractor()`; while for 2) I feel
> comfortable
> > relying on the TopicNameExtractor#toString() in `Source#toString()` impl
> > since even if users do not override this function, the default value
> > `className@hashcode` still looks fine to me.
> >
> >
> > Guozhang
> >
> >
> >
> > On Tue, Jul 3, 2018 at 11:22 PM, Matthias J. Sax 
> > wrote:
> >
> >> I just double checked the discussion thread of KIP-120 that introduced
> >> `TopologyDescription`. Back than the argument was, that using the
> >> simplest option might be sufficient because the description is mostly
> >> used for debugging.
> >>
> >> Not sure if this argument holds. It seem that people built first more
> >> sophisticated tools using TopologyDescription.
> >>
> >> Final note: if we really want to add `topicPattern()` we might want to
> >> deprecate `topic()` and replace with `Set topics()`, because a
> >> source node can take a multiple topics, too.
> >>
> >> Just adding this for completeness of context to the discussion.
> >>
> >>
> >> -Matthias
> >>
> >> On 7/3/18 11:09 PM, Matthias J. Sax wrote:
> >>> John,
> >>>
> >>> I am a little bit on the fence. In retrospective, it might have been
> >>> better to add `topic()` and `topicPattern()` to source node and return
> a
> >>> proper `Pattern` object instead of the pattern as a String.
> >>>
> >>> All other "payload" is just names and thus String naturally. From my
> >>> point of view `TopologyDescription` should represent the `Topology` in
> a
> >>> "machine readable" form plus a default "human readable" from via
> >>> `toString()` -- this does not imply that all return types should be
> >> String.
> >>>
> >>> Let me know what you think. If you agree, we could even add
> >>> `Source#topicPattern()` in another KIP.
> >>>
> >>>
> >>> -Matthias
> >>>
> >>> On 6/26/18 3:45 PM, John Roesler wrote:
>  Sorry for the late comment,
> 
>  Looking at the other pieces of TopologyDescription, I noticed that
> >> pretty
>  much all of the "payload" of these description nodes are strings.
> >> Should we
>  consider returning a string from `topicNameExtractor()` instead?
> 
>  In fact, if we did that, we could consider calling `toString()` on the
>  extractor instead of returning the class name. This would allow
> authors
> >> of
>  the extractors to provide more information about the extractor than
> just
>  its name. This might be especially useful in the case of anonymous
>  implementations.
> 
>  Thanks for the KIP,
>  -John
> 
>  On Mon, Jun 25, 2018 at 11:52 PM Ted Yu  wrote:
> 
> > My previous response was talking about the new method in
> > InternalTopologyBuilder.
> > The exception just means there is no uniform extractor for all the
> >> sinks.
> >
> > On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax <
> >> matth...@confluent.io>
> > wrote:
> >
> >> Ted,
> >>
> >> Why? Each sink can have a different TopicNameExtractor.
> >>
> >>
> >> -Matthias
> >>
> >> On 6/25/18 5:19 PM, Ted Yu wrote:
> >>> If there are different TopicNameExtractor classes from multiple
> sink
> >> nodes,
> >>> the new method should throw exception alerting user of such
> scenario.
> >>>
> >>>
> >>> On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck 
> > wrote:
> >>>
>  

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

Sounds good to me.

-Matthias

On 7/4/18 10:53 AM, Guozhang Wang wrote:
> After looked through the current TopologyDescription I think I'd want to
> combine the suggestions from John and Matthias on the API proposal. The
> motivations is that we have two relatively different functionalities
> provided from the APIs today:
> 
> 1. Each interface's public functions, like
> SourceNode#topics(), GlobalStore#source(), which returns non-String typed
> data. The hope was to let users programmatically leverage on those APIs for
> runtime checking.
> 2. Each interface's impl class also have an implicit toString() overridden
> to print the necessary information. This was designed for debugging
> purposes only during development cycles.
> 
> What we've observed so far, though, is that users leverage 2) much more
> than 1) in practice, since it is more convienent to parse strings than
> recursively calling the APIs to get non-string fields. On the other hand,
> the discussion controversy is more around 1), not 2). As for 2) people seem
> to be on the right page anyways: print the topic lists if it is not
> dynamic, or print extractor string format otherwise. For 1) above we should
> probably have all three `Set topics()`, `Pattern topicPattern()`
> and `TopicNameExtractor topicExtractor()`; while for 2) I feel comfortable
> relying on the TopicNameExtractor#toString() in `Source#toString()` impl
> since even if users do not override this function, the default value
> `className@hashcode` still looks fine to me.
> 
> 
> Guozhang
> 
> 
> 
> On Tue, Jul 3, 2018 at 11:22 PM, Matthias J. Sax 
> wrote:
> 
>> I just double checked the discussion thread of KIP-120 that introduced
>> `TopologyDescription`. Back than the argument was, that using the
>> simplest option might be sufficient because the description is mostly
>> used for debugging.
>>
>> Not sure if this argument holds. It seem that people built first more
>> sophisticated tools using TopologyDescription.
>>
>> Final note: if we really want to add `topicPattern()` we might want to
>> deprecate `topic()` and replace with `Set topics()`, because a
>> source node can take a multiple topics, too.
>>
>> Just adding this for completeness of context to the discussion.
>>
>>
>> -Matthias
>>
>> On 7/3/18 11:09 PM, Matthias J. Sax wrote:
>>> John,
>>>
>>> I am a little bit on the fence. In retrospective, it might have been
>>> better to add `topic()` and `topicPattern()` to source node and return a
>>> proper `Pattern` object instead of the pattern as a String.
>>>
>>> All other "payload" is just names and thus String naturally. From my
>>> point of view `TopologyDescription` should represent the `Topology` in a
>>> "machine readable" form plus a default "human readable" from via
>>> `toString()` -- this does not imply that all return types should be
>> String.
>>>
>>> Let me know what you think. If you agree, we could even add
>>> `Source#topicPattern()` in another KIP.
>>>
>>>
>>> -Matthias
>>>
>>> On 6/26/18 3:45 PM, John Roesler wrote:
 Sorry for the late comment,

 Looking at the other pieces of TopologyDescription, I noticed that
>> pretty
 much all of the "payload" of these description nodes are strings.
>> Should we
 consider returning a string from `topicNameExtractor()` instead?

 In fact, if we did that, we could consider calling `toString()` on the
 extractor instead of returning the class name. This would allow authors
>> of
 the extractors to provide more information about the extractor than just
 its name. This might be especially useful in the case of anonymous
 implementations.

 Thanks for the KIP,
 -John

 On Mon, Jun 25, 2018 at 11:52 PM Ted Yu  wrote:

> My previous response was talking about the new method in
> InternalTopologyBuilder.
> The exception just means there is no uniform extractor for all the
>> sinks.
>
> On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax <
>> matth...@confluent.io>
> wrote:
>
>> Ted,
>>
>> Why? Each sink can have a different TopicNameExtractor.
>>
>>
>> -Matthias
>>
>> On 6/25/18 5:19 PM, Ted Yu wrote:
>>> If there are different TopicNameExtractor classes from multiple sink
>> nodes,
>>> the new method should throw exception alerting user of such scenario.
>>>
>>>
>>> On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck 
> wrote:
>>>
 Thanks for the KIP!

 Overall I'm +1 on the KIP.   I have one question.

 The KIP states that the method "topicNameExtractor()" is added to
>> the
 InternalTopologyBuilder.java.

 It could be that I'm missing something, but wow does this work if a
> user
 has provided different TopicNameExtractor instances to multiple sink
>> nodes?

 Thanks,
 Bill



 On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang 
>> wrote:

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

After looked through the current TopologyDescription I think I'd want to
combine the suggestions from John and Matthias on the API proposal. The
motivations is that we have two relatively different functionalities
provided from the APIs today:

1. Each interface's public functions, like
SourceNode#topics(), GlobalStore#source(), which returns non-String typed
data. The hope was to let users programmatically leverage on those APIs for
runtime checking.
2. Each interface's impl class also have an implicit toString() overridden
to print the necessary information. This was designed for debugging
purposes only during development cycles.

What we've observed so far, though, is that users leverage 2) much more
than 1) in practice, since it is more convienent to parse strings than
recursively calling the APIs to get non-string fields. On the other hand,
the discussion controversy is more around 1), not 2). As for 2) people seem
to be on the right page anyways: print the topic lists if it is not
dynamic, or print extractor string format otherwise. For 1) above we should
probably have all three `Set topics()`, `Pattern topicPattern()`
and `TopicNameExtractor topicExtractor()`; while for 2) I feel comfortable
relying on the TopicNameExtractor#toString() in `Source#toString()` impl
since even if users do not override this function, the default value
`className@hashcode` still looks fine to me.


Guozhang



On Tue, Jul 3, 2018 at 11:22 PM, Matthias J. Sax 
wrote:

> I just double checked the discussion thread of KIP-120 that introduced
> `TopologyDescription`. Back than the argument was, that using the
> simplest option might be sufficient because the description is mostly
> used for debugging.
>
> Not sure if this argument holds. It seem that people built first more
> sophisticated tools using TopologyDescription.
>
> Final note: if we really want to add `topicPattern()` we might want to
> deprecate `topic()` and replace with `Set topics()`, because a
> source node can take a multiple topics, too.
>
> Just adding this for completeness of context to the discussion.
>
>
> -Matthias
>
> On 7/3/18 11:09 PM, Matthias J. Sax wrote:
> > John,
> >
> > I am a little bit on the fence. In retrospective, it might have been
> > better to add `topic()` and `topicPattern()` to source node and return a
> > proper `Pattern` object instead of the pattern as a String.
> >
> > All other "payload" is just names and thus String naturally. From my
> > point of view `TopologyDescription` should represent the `Topology` in a
> > "machine readable" form plus a default "human readable" from via
> > `toString()` -- this does not imply that all return types should be
> String.
> >
> > Let me know what you think. If you agree, we could even add
> > `Source#topicPattern()` in another KIP.
> >
> >
> > -Matthias
> >
> > On 6/26/18 3:45 PM, John Roesler wrote:
> >> Sorry for the late comment,
> >>
> >> Looking at the other pieces of TopologyDescription, I noticed that
> pretty
> >> much all of the "payload" of these description nodes are strings.
> Should we
> >> consider returning a string from `topicNameExtractor()` instead?
> >>
> >> In fact, if we did that, we could consider calling `toString()` on the
> >> extractor instead of returning the class name. This would allow authors
> of
> >> the extractors to provide more information about the extractor than just
> >> its name. This might be especially useful in the case of anonymous
> >> implementations.
> >>
> >> Thanks for the KIP,
> >> -John
> >>
> >> On Mon, Jun 25, 2018 at 11:52 PM Ted Yu  wrote:
> >>
> >>> My previous response was talking about the new method in
> >>> InternalTopologyBuilder.
> >>> The exception just means there is no uniform extractor for all the
> sinks.
> >>>
> >>> On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax <
> matth...@confluent.io>
> >>> wrote:
> >>>
>  Ted,
> 
>  Why? Each sink can have a different TopicNameExtractor.
> 
> 
>  -Matthias
> 
>  On 6/25/18 5:19 PM, Ted Yu wrote:
> > If there are different TopicNameExtractor classes from multiple sink
>  nodes,
> > the new method should throw exception alerting user of such scenario.
> >
> >
> > On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck 
> >>> wrote:
> >
> >> Thanks for the KIP!
> >>
> >> Overall I'm +1 on the KIP.   I have one question.
> >>
> >> The KIP states that the method "topicNameExtractor()" is added to
> the
> >> InternalTopologyBuilder.java.
> >>
> >> It could be that I'm missing something, but wow does this work if a
> >>> user
> >> has provided different TopicNameExtractor instances to multiple sink
>  nodes?
> >>
> >> Thanks,
> >> Bill
> >>
> >>
> >>
> >> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang 
>  wrote:
> >>
> >>> Yup I agree, generally speaking the `toString()` output is not
> >> recommended
> >>> to be relied on programmatically in user's code, 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

I just double checked the discussion thread of KIP-120 that introduced
`TopologyDescription`. Back than the argument was, that using the
simplest option might be sufficient because the description is mostly
used for debugging.

Not sure if this argument holds. It seem that people built first more
sophisticated tools using TopologyDescription.

Final note: if we really want to add `topicPattern()` we might want to
deprecate `topic()` and replace with `Set topics()`, because a
source node can take a multiple topics, too.

Just adding this for completeness of context to the discussion.


-Matthias

On 7/3/18 11:09 PM, Matthias J. Sax wrote:
> John,
> 
> I am a little bit on the fence. In retrospective, it might have been
> better to add `topic()` and `topicPattern()` to source node and return a
> proper `Pattern` object instead of the pattern as a String.
> 
> All other "payload" is just names and thus String naturally. From my
> point of view `TopologyDescription` should represent the `Topology` in a
> "machine readable" form plus a default "human readable" from via
> `toString()` -- this does not imply that all return types should be String.
> 
> Let me know what you think. If you agree, we could even add
> `Source#topicPattern()` in another KIP.
> 
> 
> -Matthias
> 
> On 6/26/18 3:45 PM, John Roesler wrote:
>> Sorry for the late comment,
>>
>> Looking at the other pieces of TopologyDescription, I noticed that pretty
>> much all of the "payload" of these description nodes are strings. Should we
>> consider returning a string from `topicNameExtractor()` instead?
>>
>> In fact, if we did that, we could consider calling `toString()` on the
>> extractor instead of returning the class name. This would allow authors of
>> the extractors to provide more information about the extractor than just
>> its name. This might be especially useful in the case of anonymous
>> implementations.
>>
>> Thanks for the KIP,
>> -John
>>
>> On Mon, Jun 25, 2018 at 11:52 PM Ted Yu  wrote:
>>
>>> My previous response was talking about the new method in
>>> InternalTopologyBuilder.
>>> The exception just means there is no uniform extractor for all the sinks.
>>>
>>> On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax 
>>> wrote:
>>>
 Ted,

 Why? Each sink can have a different TopicNameExtractor.


 -Matthias

 On 6/25/18 5:19 PM, Ted Yu wrote:
> If there are different TopicNameExtractor classes from multiple sink
 nodes,
> the new method should throw exception alerting user of such scenario.
>
>
> On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck 
>>> wrote:
>
>> Thanks for the KIP!
>>
>> Overall I'm +1 on the KIP.   I have one question.
>>
>> The KIP states that the method "topicNameExtractor()" is added to the
>> InternalTopologyBuilder.java.
>>
>> It could be that I'm missing something, but wow does this work if a
>>> user
>> has provided different TopicNameExtractor instances to multiple sink
 nodes?
>>
>> Thanks,
>> Bill
>>
>>
>>
>> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang 
 wrote:
>>
>>> Yup I agree, generally speaking the `toString()` output is not
>> recommended
>>> to be relied on programmatically in user's code, but we've observed
>>> convenience-beats-any-other-reasons again and again in development
>>> unfortunately. I think we should still not claiming it is part of the
>>> public APIs that would not be changed anyhow in the future, but just
>>> mentioning it in the wiki for people to be aware is fine.
>>>
>>>
>>> Guozhang
>>>
>>> On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax <
 matth...@confluent.io>
>>> wrote:
>>>
 Thanks for the KIP!

 I am don't have any further comments.

 For Guozhang's comment: if we mention anything about `toString()`,
>>> we
 should make explicit that `toString()` output is still not public
 contract and users should not rely on the output.

 Furhtermore, for the actual uses output, I would replace "topic:" by
 "extractor class:" to make the difference obvious.

 I am just hoping that people actually to not rely on `toString()`
>>> what
 defeats the purpose to the `TopologyDescription` class that was
 introduced to avoid the dependency... (Just a side comment, not
>>> really
 related to this KIP proposal itself).


 If there are no further comments in the next days, feel free to
>>> start
 the VOTE and open a PR.




 -Matthias

 On 6/22/18 6:04 PM, Guozhang Wang wrote:
> Thanks for writing the KIP!
>
> I'm +1 on the proposed changes over all. One minor suggestion: we
>>> should
> also mention that the `Sink#toString` will also be updated, in a
>>> way
>>> 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

John,

I am a little bit on the fence. In retrospective, it might have been
better to add `topic()` and `topicPattern()` to source node and return a
proper `Pattern` object instead of the pattern as a String.

All other "payload" is just names and thus String naturally. From my
point of view `TopologyDescription` should represent the `Topology` in a
"machine readable" form plus a default "human readable" from via
`toString()` -- this does not imply that all return types should be String.

Let me know what you think. If you agree, we could even add
`Source#topicPattern()` in another KIP.


-Matthias

On 6/26/18 3:45 PM, John Roesler wrote:
> Sorry for the late comment,
> 
> Looking at the other pieces of TopologyDescription, I noticed that pretty
> much all of the "payload" of these description nodes are strings. Should we
> consider returning a string from `topicNameExtractor()` instead?
> 
> In fact, if we did that, we could consider calling `toString()` on the
> extractor instead of returning the class name. This would allow authors of
> the extractors to provide more information about the extractor than just
> its name. This might be especially useful in the case of anonymous
> implementations.
> 
> Thanks for the KIP,
> -John
> 
> On Mon, Jun 25, 2018 at 11:52 PM Ted Yu  wrote:
> 
>> My previous response was talking about the new method in
>> InternalTopologyBuilder.
>> The exception just means there is no uniform extractor for all the sinks.
>>
>> On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax 
>> wrote:
>>
>>> Ted,
>>>
>>> Why? Each sink can have a different TopicNameExtractor.
>>>
>>>
>>> -Matthias
>>>
>>> On 6/25/18 5:19 PM, Ted Yu wrote:
 If there are different TopicNameExtractor classes from multiple sink
>>> nodes,
 the new method should throw exception alerting user of such scenario.


 On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck 
>> wrote:

> Thanks for the KIP!
>
> Overall I'm +1 on the KIP.   I have one question.
>
> The KIP states that the method "topicNameExtractor()" is added to the
> InternalTopologyBuilder.java.
>
> It could be that I'm missing something, but wow does this work if a
>> user
> has provided different TopicNameExtractor instances to multiple sink
>>> nodes?
>
> Thanks,
> Bill
>
>
>
> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang 
>>> wrote:
>
>> Yup I agree, generally speaking the `toString()` output is not
> recommended
>> to be relied on programmatically in user's code, but we've observed
>> convenience-beats-any-other-reasons again and again in development
>> unfortunately. I think we should still not claiming it is part of the
>> public APIs that would not be changed anyhow in the future, but just
>> mentioning it in the wiki for people to be aware is fine.
>>
>>
>> Guozhang
>>
>> On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax <
>>> matth...@confluent.io>
>> wrote:
>>
>>> Thanks for the KIP!
>>>
>>> I am don't have any further comments.
>>>
>>> For Guozhang's comment: if we mention anything about `toString()`,
>> we
>>> should make explicit that `toString()` output is still not public
>>> contract and users should not rely on the output.
>>>
>>> Furhtermore, for the actual uses output, I would replace "topic:" by
>>> "extractor class:" to make the difference obvious.
>>>
>>> I am just hoping that people actually to not rely on `toString()`
>> what
>>> defeats the purpose to the `TopologyDescription` class that was
>>> introduced to avoid the dependency... (Just a side comment, not
>> really
>>> related to this KIP proposal itself).
>>>
>>>
>>> If there are no further comments in the next days, feel free to
>> start
>>> the VOTE and open a PR.
>>>
>>>
>>>
>>>
>>> -Matthias
>>>
>>> On 6/22/18 6:04 PM, Guozhang Wang wrote:
 Thanks for writing the KIP!

 I'm +1 on the proposed changes over all. One minor suggestion: we
>> should
 also mention that the `Sink#toString` will also be updated, in a
>> way
>> that
 if `topic()` returns null, use the other call, etc. This is because
 although we do not explicitly state the following logic as public
>>> protocols:

 ```

 "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
 nodeNames(predecessors);


 ```

 There are already some users that rely on
>> `topology.describe().toString(
>>> )`
 to have runtime checks on the returned string values, so changing
> this
 means that their app will break and hence need to make code
>> changes.

 Guozhang

 On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep <
>> nishanth...@gmail.com

 wrote:

> Hello 

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[John Roesler]

Sorry for the late comment,

Looking at the other pieces of TopologyDescription, I noticed that pretty
much all of the "payload" of these description nodes are strings. Should we
consider returning a string from `topicNameExtractor()` instead?

In fact, if we did that, we could consider calling `toString()` on the
extractor instead of returning the class name. This would allow authors of
the extractors to provide more information about the extractor than just
its name. This might be especially useful in the case of anonymous
implementations.

Thanks for the KIP,
-John

On Mon, Jun 25, 2018 at 11:52 PM Ted Yu  wrote:

> My previous response was talking about the new method in
> InternalTopologyBuilder.
> The exception just means there is no uniform extractor for all the sinks.
>
> On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax 
> wrote:
>
> > Ted,
> >
> > Why? Each sink can have a different TopicNameExtractor.
> >
> >
> > -Matthias
> >
> > On 6/25/18 5:19 PM, Ted Yu wrote:
> > > If there are different TopicNameExtractor classes from multiple sink
> > nodes,
> > > the new method should throw exception alerting user of such scenario.
> > >
> > >
> > > On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck 
> wrote:
> > >
> > >> Thanks for the KIP!
> > >>
> > >> Overall I'm +1 on the KIP.   I have one question.
> > >>
> > >> The KIP states that the method "topicNameExtractor()" is added to the
> > >> InternalTopologyBuilder.java.
> > >>
> > >> It could be that I'm missing something, but wow does this work if a
> user
> > >> has provided different TopicNameExtractor instances to multiple sink
> > nodes?
> > >>
> > >> Thanks,
> > >> Bill
> > >>
> > >>
> > >>
> > >> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang 
> > wrote:
> > >>
> > >>> Yup I agree, generally speaking the `toString()` output is not
> > >> recommended
> > >>> to be relied on programmatically in user's code, but we've observed
> > >>> convenience-beats-any-other-reasons again and again in development
> > >>> unfortunately. I think we should still not claiming it is part of the
> > >>> public APIs that would not be changed anyhow in the future, but just
> > >>> mentioning it in the wiki for people to be aware is fine.
> > >>>
> > >>>
> > >>> Guozhang
> > >>>
> > >>> On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax <
> > matth...@confluent.io>
> > >>> wrote:
> > >>>
> >  Thanks for the KIP!
> > 
> >  I am don't have any further comments.
> > 
> >  For Guozhang's comment: if we mention anything about `toString()`,
> we
> >  should make explicit that `toString()` output is still not public
> >  contract and users should not rely on the output.
> > 
> >  Furhtermore, for the actual uses output, I would replace "topic:" by
> >  "extractor class:" to make the difference obvious.
> > 
> >  I am just hoping that people actually to not rely on `toString()`
> what
> >  defeats the purpose to the `TopologyDescription` class that was
> >  introduced to avoid the dependency... (Just a side comment, not
> really
> >  related to this KIP proposal itself).
> > 
> > 
> >  If there are no further comments in the next days, feel free to
> start
> >  the VOTE and open a PR.
> > 
> > 
> > 
> > 
> >  -Matthias
> > 
> >  On 6/22/18 6:04 PM, Guozhang Wang wrote:
> > > Thanks for writing the KIP!
> > >
> > > I'm +1 on the proposed changes over all. One minor suggestion: we
> > >>> should
> > > also mention that the `Sink#toString` will also be updated, in a
> way
> > >>> that
> > > if `topic()` returns null, use the other call, etc. This is because
> > > although we do not explicitly state the following logic as public
> >  protocols:
> > >
> > > ```
> > >
> > > "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> > > nodeNames(predecessors);
> > >
> > >
> > > ```
> > >
> > > There are already some users that rely on
> > >>> `topology.describe().toString(
> >  )`
> > > to have runtime checks on the returned string values, so changing
> > >> this
> > > means that their app will break and hence need to make code
> changes.
> > >
> > > Guozhang
> > >
> > > On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep <
> > >>> nishanth...@gmail.com
> > >
> > > wrote:
> > >
> > >> Hello Everyone,
> > >>
> > >> I have created a new KIP to discuss extending TopologyDescription.
> > >> You
> >  can
> > >> find it here:
> > >>
> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
> > >>
> > >> Please provide any feedback that you might have.
> > >>
> > >> Best,
> > >> Nishanth Pradeep
> > >>
> > >
> > >
> > >
> > 
> > 
> > >>>
> > >>>
> > >>> --
> > >>> -- Guozhang
> > >>>
> > >>
> > >
> >
> >
>

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Ted Yu]

My previous response was talking about the new method in
InternalTopologyBuilder.
The exception just means there is no uniform extractor for all the sinks.

On Mon, Jun 25, 2018 at 8:02 PM, Matthias J. Sax 
wrote:

> Ted,
>
> Why? Each sink can have a different TopicNameExtractor.
>
>
> -Matthias
>
> On 6/25/18 5:19 PM, Ted Yu wrote:
> > If there are different TopicNameExtractor classes from multiple sink
> nodes,
> > the new method should throw exception alerting user of such scenario.
> >
> >
> > On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck  wrote:
> >
> >> Thanks for the KIP!
> >>
> >> Overall I'm +1 on the KIP.   I have one question.
> >>
> >> The KIP states that the method "topicNameExtractor()" is added to the
> >> InternalTopologyBuilder.java.
> >>
> >> It could be that I'm missing something, but wow does this work if a user
> >> has provided different TopicNameExtractor instances to multiple sink
> nodes?
> >>
> >> Thanks,
> >> Bill
> >>
> >>
> >>
> >> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang 
> wrote:
> >>
> >>> Yup I agree, generally speaking the `toString()` output is not
> >> recommended
> >>> to be relied on programmatically in user's code, but we've observed
> >>> convenience-beats-any-other-reasons again and again in development
> >>> unfortunately. I think we should still not claiming it is part of the
> >>> public APIs that would not be changed anyhow in the future, but just
> >>> mentioning it in the wiki for people to be aware is fine.
> >>>
> >>>
> >>> Guozhang
> >>>
> >>> On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax <
> matth...@confluent.io>
> >>> wrote:
> >>>
>  Thanks for the KIP!
> 
>  I am don't have any further comments.
> 
>  For Guozhang's comment: if we mention anything about `toString()`, we
>  should make explicit that `toString()` output is still not public
>  contract and users should not rely on the output.
> 
>  Furhtermore, for the actual uses output, I would replace "topic:" by
>  "extractor class:" to make the difference obvious.
> 
>  I am just hoping that people actually to not rely on `toString()` what
>  defeats the purpose to the `TopologyDescription` class that was
>  introduced to avoid the dependency... (Just a side comment, not really
>  related to this KIP proposal itself).
> 
> 
>  If there are no further comments in the next days, feel free to start
>  the VOTE and open a PR.
> 
> 
> 
> 
>  -Matthias
> 
>  On 6/22/18 6:04 PM, Guozhang Wang wrote:
> > Thanks for writing the KIP!
> >
> > I'm +1 on the proposed changes over all. One minor suggestion: we
> >>> should
> > also mention that the `Sink#toString` will also be updated, in a way
> >>> that
> > if `topic()` returns null, use the other call, etc. This is because
> > although we do not explicitly state the following logic as public
>  protocols:
> >
> > ```
> >
> > "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> > nodeNames(predecessors);
> >
> >
> > ```
> >
> > There are already some users that rely on
> >>> `topology.describe().toString(
>  )`
> > to have runtime checks on the returned string values, so changing
> >> this
> > means that their app will break and hence need to make code changes.
> >
> > Guozhang
> >
> > On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep <
> >>> nishanth...@gmail.com
> >
> > wrote:
> >
> >> Hello Everyone,
> >>
> >> I have created a new KIP to discuss extending TopologyDescription.
> >> You
>  can
> >> find it here:
> >>
> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
> >>
> >> Please provide any feedback that you might have.
> >>
> >> Best,
> >> Nishanth Pradeep
> >>
> >
> >
> >
> 
> 
> >>>
> >>>
> >>> --
> >>> -- Guozhang
> >>>
> >>
> >
>
>

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

Ted,

Why? Each sink can have a different TopicNameExtractor.


-Matthias

On 6/25/18 5:19 PM, Ted Yu wrote:
> If there are different TopicNameExtractor classes from multiple sink nodes,
> the new method should throw exception alerting user of such scenario.
> 
> 
> On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck  wrote:
> 
>> Thanks for the KIP!
>>
>> Overall I'm +1 on the KIP.   I have one question.
>>
>> The KIP states that the method "topicNameExtractor()" is added to the
>> InternalTopologyBuilder.java.
>>
>> It could be that I'm missing something, but wow does this work if a user
>> has provided different TopicNameExtractor instances to multiple sink nodes?
>>
>> Thanks,
>> Bill
>>
>>
>>
>> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang  wrote:
>>
>>> Yup I agree, generally speaking the `toString()` output is not
>> recommended
>>> to be relied on programmatically in user's code, but we've observed
>>> convenience-beats-any-other-reasons again and again in development
>>> unfortunately. I think we should still not claiming it is part of the
>>> public APIs that would not be changed anyhow in the future, but just
>>> mentioning it in the wiki for people to be aware is fine.
>>>
>>>
>>> Guozhang
>>>
>>> On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax 
>>> wrote:
>>>
 Thanks for the KIP!

 I am don't have any further comments.

 For Guozhang's comment: if we mention anything about `toString()`, we
 should make explicit that `toString()` output is still not public
 contract and users should not rely on the output.

 Furhtermore, for the actual uses output, I would replace "topic:" by
 "extractor class:" to make the difference obvious.

 I am just hoping that people actually to not rely on `toString()` what
 defeats the purpose to the `TopologyDescription` class that was
 introduced to avoid the dependency... (Just a side comment, not really
 related to this KIP proposal itself).


 If there are no further comments in the next days, feel free to start
 the VOTE and open a PR.




 -Matthias

 On 6/22/18 6:04 PM, Guozhang Wang wrote:
> Thanks for writing the KIP!
>
> I'm +1 on the proposed changes over all. One minor suggestion: we
>>> should
> also mention that the `Sink#toString` will also be updated, in a way
>>> that
> if `topic()` returns null, use the other call, etc. This is because
> although we do not explicitly state the following logic as public
 protocols:
>
> ```
>
> "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> nodeNames(predecessors);
>
>
> ```
>
> There are already some users that rely on
>>> `topology.describe().toString(
 )`
> to have runtime checks on the returned string values, so changing
>> this
> means that their app will break and hence need to make code changes.
>
> Guozhang
>
> On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep <
>>> nishanth...@gmail.com
>
> wrote:
>
>> Hello Everyone,
>>
>> I have created a new KIP to discuss extending TopologyDescription.
>> You
 can
>> find it here:
>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
>>
>> Please provide any feedback that you might have.
>>
>> Best,
>> Nishanth Pradeep
>>
>
>
>


>>>
>>>
>>> --
>>> -- Guozhang
>>>
>>
> 



signature.asc
Description: OpenPGP digital signature

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Ted Yu]

If there are different TopicNameExtractor classes from multiple sink nodes,
the new method should throw exception alerting user of such scenario.


On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck  wrote:

> Thanks for the KIP!
>
> Overall I'm +1 on the KIP.   I have one question.
>
> The KIP states that the method "topicNameExtractor()" is added to the
> InternalTopologyBuilder.java.
>
> It could be that I'm missing something, but wow does this work if a user
> has provided different TopicNameExtractor instances to multiple sink nodes?
>
> Thanks,
> Bill
>
>
>
> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang  wrote:
>
> > Yup I agree, generally speaking the `toString()` output is not
> recommended
> > to be relied on programmatically in user's code, but we've observed
> > convenience-beats-any-other-reasons again and again in development
> > unfortunately. I think we should still not claiming it is part of the
> > public APIs that would not be changed anyhow in the future, but just
> > mentioning it in the wiki for people to be aware is fine.
> >
> >
> > Guozhang
> >
> > On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax 
> > wrote:
> >
> > > Thanks for the KIP!
> > >
> > > I am don't have any further comments.
> > >
> > > For Guozhang's comment: if we mention anything about `toString()`, we
> > > should make explicit that `toString()` output is still not public
> > > contract and users should not rely on the output.
> > >
> > > Furhtermore, for the actual uses output, I would replace "topic:" by
> > > "extractor class:" to make the difference obvious.
> > >
> > > I am just hoping that people actually to not rely on `toString()` what
> > > defeats the purpose to the `TopologyDescription` class that was
> > > introduced to avoid the dependency... (Just a side comment, not really
> > > related to this KIP proposal itself).
> > >
> > >
> > > If there are no further comments in the next days, feel free to start
> > > the VOTE and open a PR.
> > >
> > >
> > >
> > >
> > > -Matthias
> > >
> > > On 6/22/18 6:04 PM, Guozhang Wang wrote:
> > > > Thanks for writing the KIP!
> > > >
> > > > I'm +1 on the proposed changes over all. One minor suggestion: we
> > should
> > > > also mention that the `Sink#toString` will also be updated, in a way
> > that
> > > > if `topic()` returns null, use the other call, etc. This is because
> > > > although we do not explicitly state the following logic as public
> > > protocols:
> > > >
> > > > ```
> > > >
> > > > "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> > > > nodeNames(predecessors);
> > > >
> > > >
> > > > ```
> > > >
> > > > There are already some users that rely on
> > `topology.describe().toString(
> > > )`
> > > > to have runtime checks on the returned string values, so changing
> this
> > > > means that their app will break and hence need to make code changes.
> > > >
> > > > Guozhang
> > > >
> > > > On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep <
> > nishanth...@gmail.com
> > > >
> > > > wrote:
> > > >
> > > >> Hello Everyone,
> > > >>
> > > >> I have created a new KIP to discuss extending TopologyDescription.
> You
> > > can
> > > >> find it here:
> > > >>
> > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > >> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
> > > >>
> > > >> Please provide any feedback that you might have.
> > > >>
> > > >> Best,
> > > >> Nishanth Pradeep
> > > >>
> > > >
> > > >
> > > >
> > >
> > >
> >
> >
> > --
> > -- Guozhang
> >
>

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

Good catch. I think the proposed change is to add that function in
InternalTopologyBuilder#Sink class.



Guozhang

On Mon, Jun 25, 2018 at 2:23 PM, Bill Bejeck  wrote:

> Thanks for the KIP!
>
> Overall I'm +1 on the KIP.   I have one question.
>
> The KIP states that the method "topicNameExtractor()" is added to the
> InternalTopologyBuilder.java.
>
> It could be that I'm missing something, but wow does this work if a user
> has provided different TopicNameExtractor instances to multiple sink nodes?
>
> Thanks,
> Bill
>
>
>
> On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang  wrote:
>
> > Yup I agree, generally speaking the `toString()` output is not
> recommended
> > to be relied on programmatically in user's code, but we've observed
> > convenience-beats-any-other-reasons again and again in development
> > unfortunately. I think we should still not claiming it is part of the
> > public APIs that would not be changed anyhow in the future, but just
> > mentioning it in the wiki for people to be aware is fine.
> >
> >
> > Guozhang
> >
> > On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax 
> > wrote:
> >
> > > Thanks for the KIP!
> > >
> > > I am don't have any further comments.
> > >
> > > For Guozhang's comment: if we mention anything about `toString()`, we
> > > should make explicit that `toString()` output is still not public
> > > contract and users should not rely on the output.
> > >
> > > Furhtermore, for the actual uses output, I would replace "topic:" by
> > > "extractor class:" to make the difference obvious.
> > >
> > > I am just hoping that people actually to not rely on `toString()` what
> > > defeats the purpose to the `TopologyDescription` class that was
> > > introduced to avoid the dependency... (Just a side comment, not really
> > > related to this KIP proposal itself).
> > >
> > >
> > > If there are no further comments in the next days, feel free to start
> > > the VOTE and open a PR.
> > >
> > >
> > >
> > >
> > > -Matthias
> > >
> > > On 6/22/18 6:04 PM, Guozhang Wang wrote:
> > > > Thanks for writing the KIP!
> > > >
> > > > I'm +1 on the proposed changes over all. One minor suggestion: we
> > should
> > > > also mention that the `Sink#toString` will also be updated, in a way
> > that
> > > > if `topic()` returns null, use the other call, etc. This is because
> > > > although we do not explicitly state the following logic as public
> > > protocols:
> > > >
> > > > ```
> > > >
> > > > "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> > > > nodeNames(predecessors);
> > > >
> > > >
> > > > ```
> > > >
> > > > There are already some users that rely on
> > `topology.describe().toString(
> > > )`
> > > > to have runtime checks on the returned string values, so changing
> this
> > > > means that their app will break and hence need to make code changes.
> > > >
> > > > Guozhang
> > > >
> > > > On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep <
> > nishanth...@gmail.com
> > > >
> > > > wrote:
> > > >
> > > >> Hello Everyone,
> > > >>
> > > >> I have created a new KIP to discuss extending TopologyDescription.
> You
> > > can
> > > >> find it here:
> > > >>
> > > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > >> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
> > > >>
> > > >> Please provide any feedback that you might have.
> > > >>
> > > >> Best,
> > > >> Nishanth Pradeep
> > > >>
> > > >
> > > >
> > > >
> > >
> > >
> >
> >
> > --
> > -- Guozhang
> >
>



-- 
-- Guozhang

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Bill Bejeck]

Thanks for the KIP!

Overall I'm +1 on the KIP.   I have one question.

The KIP states that the method "topicNameExtractor()" is added to the
InternalTopologyBuilder.java.

It could be that I'm missing something, but wow does this work if a user
has provided different TopicNameExtractor instances to multiple sink nodes?

Thanks,
Bill



On Mon, Jun 25, 2018 at 1:25 PM Guozhang Wang  wrote:

> Yup I agree, generally speaking the `toString()` output is not recommended
> to be relied on programmatically in user's code, but we've observed
> convenience-beats-any-other-reasons again and again in development
> unfortunately. I think we should still not claiming it is part of the
> public APIs that would not be changed anyhow in the future, but just
> mentioning it in the wiki for people to be aware is fine.
>
>
> Guozhang
>
> On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax 
> wrote:
>
> > Thanks for the KIP!
> >
> > I am don't have any further comments.
> >
> > For Guozhang's comment: if we mention anything about `toString()`, we
> > should make explicit that `toString()` output is still not public
> > contract and users should not rely on the output.
> >
> > Furhtermore, for the actual uses output, I would replace "topic:" by
> > "extractor class:" to make the difference obvious.
> >
> > I am just hoping that people actually to not rely on `toString()` what
> > defeats the purpose to the `TopologyDescription` class that was
> > introduced to avoid the dependency... (Just a side comment, not really
> > related to this KIP proposal itself).
> >
> >
> > If there are no further comments in the next days, feel free to start
> > the VOTE and open a PR.
> >
> >
> >
> >
> > -Matthias
> >
> > On 6/22/18 6:04 PM, Guozhang Wang wrote:
> > > Thanks for writing the KIP!
> > >
> > > I'm +1 on the proposed changes over all. One minor suggestion: we
> should
> > > also mention that the `Sink#toString` will also be updated, in a way
> that
> > > if `topic()` returns null, use the other call, etc. This is because
> > > although we do not explicitly state the following logic as public
> > protocols:
> > >
> > > ```
> > >
> > > "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> > > nodeNames(predecessors);
> > >
> > >
> > > ```
> > >
> > > There are already some users that rely on
> `topology.describe().toString(
> > )`
> > > to have runtime checks on the returned string values, so changing this
> > > means that their app will break and hence need to make code changes.
> > >
> > > Guozhang
> > >
> > > On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep <
> nishanth...@gmail.com
> > >
> > > wrote:
> > >
> > >> Hello Everyone,
> > >>
> > >> I have created a new KIP to discuss extending TopologyDescription. You
> > can
> > >> find it here:
> > >>
> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > >> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
> > >>
> > >> Please provide any feedback that you might have.
> > >>
> > >> Best,
> > >> Nishanth Pradeep
> > >>
> > >
> > >
> > >
> >
> >
>
>
> --
> -- Guozhang
>

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

Yup I agree, generally speaking the `toString()` output is not recommended
to be relied on programmatically in user's code, but we've observed
convenience-beats-any-other-reasons again and again in development
unfortunately. I think we should still not claiming it is part of the
public APIs that would not be changed anyhow in the future, but just
mentioning it in the wiki for people to be aware is fine.


Guozhang

On Sun, Jun 24, 2018 at 5:01 PM, Matthias J. Sax 
wrote:

> Thanks for the KIP!
>
> I am don't have any further comments.
>
> For Guozhang's comment: if we mention anything about `toString()`, we
> should make explicit that `toString()` output is still not public
> contract and users should not rely on the output.
>
> Furhtermore, for the actual uses output, I would replace "topic:" by
> "extractor class:" to make the difference obvious.
>
> I am just hoping that people actually to not rely on `toString()` what
> defeats the purpose to the `TopologyDescription` class that was
> introduced to avoid the dependency... (Just a side comment, not really
> related to this KIP proposal itself).
>
>
> If there are no further comments in the next days, feel free to start
> the VOTE and open a PR.
>
>
>
>
> -Matthias
>
> On 6/22/18 6:04 PM, Guozhang Wang wrote:
> > Thanks for writing the KIP!
> >
> > I'm +1 on the proposed changes over all. One minor suggestion: we should
> > also mention that the `Sink#toString` will also be updated, in a way that
> > if `topic()` returns null, use the other call, etc. This is because
> > although we do not explicitly state the following logic as public
> protocols:
> >
> > ```
> >
> > "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> > nodeNames(predecessors);
> >
> >
> > ```
> >
> > There are already some users that rely on `topology.describe().toString(
> )`
> > to have runtime checks on the returned string values, so changing this
> > means that their app will break and hence need to make code changes.
> >
> > Guozhang
> >
> > On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep  >
> > wrote:
> >
> >> Hello Everyone,
> >>
> >> I have created a new KIP to discuss extending TopologyDescription. You
> can
> >> find it here:
> >>
> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> >> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
> >>
> >> Please provide any feedback that you might have.
> >>
> >> Best,
> >> Nishanth Pradeep
> >>
> >
> >
> >
>
>


-- 
-- Guozhang

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Matthias J. Sax]

Thanks for the KIP!

I am don't have any further comments.

For Guozhang's comment: if we mention anything about `toString()`, we
should make explicit that `toString()` output is still not public
contract and users should not rely on the output.

Furhtermore, for the actual uses output, I would replace "topic:" by
"extractor class:" to make the difference obvious.

I am just hoping that people actually to not rely on `toString()` what
defeats the purpose to the `TopologyDescription` class that was
introduced to avoid the dependency... (Just a side comment, not really
related to this KIP proposal itself).


If there are no further comments in the next days, feel free to start
the VOTE and open a PR.




-Matthias

On 6/22/18 6:04 PM, Guozhang Wang wrote:
> Thanks for writing the KIP!
> 
> I'm +1 on the proposed changes over all. One minor suggestion: we should
> also mention that the `Sink#toString` will also be updated, in a way that
> if `topic()` returns null, use the other call, etc. This is because
> although we do not explicitly state the following logic as public protocols:
> 
> ```
> 
> "Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
> nodeNames(predecessors);
> 
> 
> ```
> 
> There are already some users that rely on `topology.describe().toString()`
> to have runtime checks on the returned string values, so changing this
> means that their app will break and hence need to make code changes.
> 
> Guozhang
> 
> On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep 
> wrote:
> 
>> Hello Everyone,
>>
>> I have created a new KIP to discuss extending TopologyDescription. You can
>> find it here:
>>
>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
>> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
>>
>> Please provide any feedback that you might have.
>>
>> Best,
>> Nishanth Pradeep
>>
> 
> 
> 



signature.asc
Description: OpenPGP digital signature

Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription

[Guozhang Wang]

Thanks for writing the KIP!

I'm +1 on the proposed changes over all. One minor suggestion: we should
also mention that the `Sink#toString` will also be updated, in a way that
if `topic()` returns null, use the other call, etc. This is because
although we do not explicitly state the following logic as public protocols:

```

"Sink: " + name + " (topic: " + topic() + ")\n  <-- " +
nodeNames(predecessors);


```

There are already some users that rely on `topology.describe().toString()`
to have runtime checks on the returned string values, so changing this
means that their app will break and hence need to make code changes.

Guozhang

On Wed, Jun 20, 2018 at 7:20 PM, Nishanth Pradeep 
wrote:

> Hello Everyone,
>
> I have created a new KIP to discuss extending TopologyDescription. You can
> find it here:
>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> 321%3A+Add+method+to+get+TopicNameExtractor+in+TopologyDescription
>
> Please provide any feedback that you might have.
>
> Best,
> Nishanth Pradeep
>



-- 
-- Guozhang