Given that there are still some open questions about the builder, maybe we 
should put it in a separate KIP?

best,
Colin


On Fri, May 10, 2019, at 09:00, Ryanne Dolan wrote:
> +1 (non-binding) for the core feature, but I could take or leave the
> builder.
> 
> Ryanne
> 
> On Fri, May 10, 2019 at 10:43 AM Almog Gavra <al...@confluent.io> wrote:
> 
> > @Ismael - I agree that the methods are a little random. They were just
> > ported from what's currently in the connect builder. I think a better
> > option might be to keep the connect builder around and have extend from
> > this builder, and make this builder only implement the "critical" methods
> > (e.g. replicas/partitions/config). (cc Randall)
> >
> > Is passing optionals in the constructor something that's common in AK? I
> > think my preference would be toward the builder so that it's easier to
> > extend as Randall mentioned.
> >
> > Almog
> >
> > On Fri, May 10, 2019 at 8:17 AM Ismael Juma <ism...@juma.me.uk> wrote:
> >
> > > The current builder includes random methods like uncleanLeaderElection.
> > > That doesn't make sense to me since it's a topic config (and we don't
> > > include methods for other topic configs). Also, I'm not sure about the
> > > naming convention, should we have a `with` prefix? It would be good to
> > > check existing builders in `clients` if any exist for what they're doing.
> > >
> > > If we didn't add a builder, another option would be a single new
> > > constructor:
> > >
> > > public NewTopic(String name, Optional<Integer> numPartitions,
> > > Optional<Short> replicationFactor)
> > >
> > > Ismael
> > >
> > > On Thu, May 9, 2019 at 3:38 PM Almog Gavra <al...@confluent.io> wrote:
> > >
> > > > Thanks Colin! Since the discussion around the builder is here I'll copy
> > > > over my comment from the discuss thread:
> > > >
> > > > If we want the flexibility that the builder provides we would need to
> > add
> > > > three constructors:
> > > > - no partitions/replicas
> > > > - just partitions
> > > > - just replicas
> > > >
> > > > I see good use cases for the first two - the third (just replicas)
> > seems
> > > > less necessary but complicates the API a bit (you have to differentiate
> > > > NewTopic(int) with NewTopic(short) or something like that). If we're
> > > happy
> > > > with a KIP that covers just the first two then I can remove the builder
> > > to
> > > > simplify things. Otherwise, I think the builder is an important
> > addition.
> > > >
> > > > Thoughts?
> > > >
> > > > On Thu, May 9, 2019 at 2:50 PM Colin McCabe <cmcc...@apache.org>
> > wrote:
> > > >
> > > > > +1 (binding).
> > > > >
> > > > > Re: the builder discussion.  I don't feel strongly either way-- the
> > > > > builder sketched out in the KIP looks reasonable, but I can also
> > > > understand
> > > > > Ismael's argument for keeping the KIP minimal.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Thu, May 9, 2019, at 08:09, Randall Hauch wrote:
> > > > > > I'm fine with simplifying the KIP by removing the Builder (which
> > > seems
> > > > > > ancillary), or keeping the KIP as-is. I'll wait to vote until Almog
> > > > says
> > > > > > which way he'd like to proceed.
> > > > > >
> > > > > > On Thu, May 9, 2019 at 9:45 AM Ismael Juma <ism...@juma.me.uk>
> > > wrote:
> > > > > >
> > > > > > > Hi Almog,
> > > > > > >
> > > > > > > Adding a Builder seems unrelated to this change. Do we need it?
> > > Given
> > > > > the
> > > > > > > imminent KIP deadline, I'd keep it simple and just have the
> > > > constructor
> > > > > > > with just the name parameter.
> > > > > > >
> > > > > > > Ismael
> > > > > > >
> > > > > > > On Thu, May 2, 2019 at 1:58 AM Mickael Maison <
> > > > > mickael.mai...@gmail.com>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > I was planning to write a KIP for the exact same feature!
> > > > > > > > +1 (non binding)
> > > > > > > >
> > > > > > > > Thanks for the KIP
> > > > > > > >
> > > > > > > > On Wed, May 1, 2019 at 7:24 PM Almog Gavra <al...@confluent.io
> > >
> > > > > wrote:
> > > > > > > > >
> > > > > > > > > Hello Everyone!
> > > > > > > > >
> > > > > > > > > Kicking off the voting for
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-464%3A+Defaults+for+AdminClient%23createTopic
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > You can see discussion thread here (please respond with
> > > > > suggestions on
> > > > > > > > that
> > > > > > > > > thread):
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
> > https://lists.apache.org/thread.html/c0adfd2457e5984be7471fe6ade8a94d52c647356c81c039445d6b34@%3Cdev.kafka.apache.org%3E
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Cheers,
> > > > > > > > > Almog
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>

Reply via email to