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 > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > >> > > > > > >
