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
