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