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