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
