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
>>>>>
>>>>
>>>
>>>
>>
>
>
> 
signature.asc
Description: OpenPGP digital signature
