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
