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 <nishanth...@gmail.com> wrote: > I have updated the KIP > <https://cwiki.apache.org/confluence/display/KAFKA/KIP-321%3A+Update+ > 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 <wangg...@gmail.com> 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 <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 > > > -- -- Guozhang
