I'm happy pulling it out into a separate KIP to target the discussion. This one can just introduce the "default" constructor for no partitions or replicas since we'll need that one whether or not we add the builder.
Updated the KIP moving the builder to a section in "Rejected Alternatives - Follow Up KIP" - @Randall since your +1 was for the KIP with builder I want to make sure that is okay with you. On Fri, May 10, 2019 at 9:14 AM Colin McCabe <cmcc...@apache.org> wrote: > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
