kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Bejeck <bbej...@gmail.com>
Subject Re: [Discuss] KIP-321: Add method to get TopicNameExtractor in TopologyDescription
Date Mon, 25 Jun 2018 21:23:54 GMT
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 <wangguoz@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 <matthias@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 <
> nishanthp21@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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message