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 <nishanth...@gmail.com>
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 <matth...@confluent.io>
> 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<String>", as Matthias mentioned it is a compatibility
> > >> breaking
> > >>>>>>> change.
> > >>>>>>>
> > >>>>>>> That's why I was proposing add a new function like "Set<String>
> > >>>>>>> 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<String> topicList() /* or Set<String> 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<String> 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<String> 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
> > >>>>>>>>>>> <https://cwiki.apache.org/confluence/display/KAFKA/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<String> for
> > >>>>>>>>> Source#topics().
> > >>>>>>>>>>>
> > >>>>>>>>>>> Set<String> 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 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 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<String> 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<String>
> > >>>>>> 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 <
> > >>>>>>> yuzhih...@gmail.com
> > >>>>>>>>>
> > >>>>>>>>>>>>> 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 <
> > >>>>>>>>>>>> bbej...@gmail.com>
> > >>>>>>>>>>>>>>>>>>>> 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 <
> > >>>>>>>>>>>>> wangg...@gmail.com
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>>> 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
> > >>>>>>>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> --
> > >>>>>>>>>>>> -- Guozhang
> > >>>>>>>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>> -- Guozhang
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> --
> > >>>>>>> -- Guozhang
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> -- Guozhang
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>
> > >
> > >
> > >
> >
> >
>
--
-- Guozhang
