I wrote the `NewTopicBuilder` in Connect, and it was simply a convenience
to more easily set some of the frequently-used properties and the # of
partitions and replicas for the new topic in the same way. An example is:
NewTopic topicDescription = TopicAdmin.defineTopic(topic).
compacted().
partitions(1).
replicationFactor(3).
build();
Arguably it should have been added to clients from the beginning. So I'm
fine with that being moved to clients, as long as Connect is changed to use
the new clients class. However, even though Connect's `NewTopicBuilder` is
in the runtime and technically not part of the public API, things like this
still tend to get reused elsewhere. Let's keep the Connect
`NewTopicBuilder` but deprecate it and have it extend the one in clients.
The `TopicAdmin` class in Connect can then refer to the new one in clients.
The KIP now talks about having a constructor for the builder:
NewTopic myTopic = new
NewTopicBuilder(name).compacted().partitions(1).replicationFactor(3).build();
How about adding the builder to the NewTopic class itself:
NewTopic myTopic =
NewTopic.build(name).compacted().partitions(1).replicationFactor(3).build();
This is a bit shorter, a bit easier to read (no "new New..."), and more
discoverable since anyone looking at the NewTopic source or JavaDoc will
maybe notice it.
Randall
On Thu, May 2, 2019 at 8:56 AM Almog Gavra <al...@confluent.io> wrote:
> Sure thing, added more detail to the KIP! To clarify, the plan is to move
> an existing API from one package to another (NewTopicBuilder exists in the
> connect.runtime package) leaving the old in place for compatibility and
> deprecating it.
>
> I'm happy to hear thoughts on whether we should (a) move it to the same
> package in a new module so that we don't need to deprecate it or (b) take
> this opportunity to change any of the APIs.
>
> On Thu, May 2, 2019 at 8:22 AM Ismael Juma <isma...@gmail.com> wrote:
>
> > If you are adding new API, you need to specify it all in the KIP.
> >
> > Ismael
> >
> > On Thu, May 2, 2019, 8:04 AM Almog Gavra <al...@confluent.io> wrote:
> >
> > > I think that sounds reasonable - I updated the KIP and I will remove
> the
> > > constructor that takes in only partitions.
> > >
> > > On Thu, May 2, 2019 at 4:44 AM Andy Coates <a...@confluent.io> wrote:
> > >
> > > > Rather than adding overloaded constructors, which can lead to API
> > bloat,
> > > > how about using a builder pattern?
> > > >
> > > > I see it’s already got some constructor overloading, but we could
> add a
> > > > single new constructor that takes just the name, and support
> everything
> > > > else being set via builder methods.
> > > >
> > > > This would result in a better long term api as the number of options
> > > > increases.
> > > >
> > > > Sent from my iPhone
> > > >
> > > > > On 30 Apr 2019, at 16:28, Almog Gavra <al...@confluent.io> wrote:
> > > > >
> > > > > Hello Everyone,
> > > > >
> > > > > I'd like to start a discussion on KIP-464:
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-464%3A+Default+Replication+Factor+for+AdminClient%23createTopic
> > > > >
> > > > > It's about allowing users of the AdminClient to supply only a
> > partition
> > > > > count and to use the default replication factor configured by the
> > kafka
> > > > > cluster. Happy to receive any and all feedback!
> > > > >
> > > > > Cheers,
> > > > > Almog
> > > >
> > >
> >
>
