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