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 >
