Good catch. I think the proposed change is to add that function in InternalTopologyBuilder#Sink class.
Guozhang 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 > > > -- -- Guozhang
