Thanks, Almog. +1 (binding) for this simpler KIP.
On Fri, May 10, 2019 at 11:37 AM Manikumar <manikumar.re...@gmail.com> wrote: > Hi Almog, > > +1 (binding), Thanks for the KIP. > > Thanks, > Manikumar > > On Fri, May 10, 2019 at 10:02 PM Almog Gavra <al...@confluent.io> wrote: > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
