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 >
