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