Hello everyone - the PR is out and ready to review! https://github.com/apache/kafka/pull/6728/
On Fri, May 10, 2019 at 11:57 AM Almog Gavra <al...@confluent.io> wrote: > Thanks everyone for the comments and discussion! Closing the voting out > for this KIP: > > * 4 Binding (Randall, Manikumar, Colin, Gwen) > * 2 Non-Binding (Ryanne, Mickael) > > Cheers, > Almog > > > On Fri, May 10, 2019 at 11:55 AM Gwen Shapira <g...@confluent.io> wrote: > >> +1 (binding) >> >> On Fri, May 10, 2019 at 9:32 AM 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 >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> >> >> -- >> *Gwen Shapira* >> Product Manager | Confluent >> 650.450.2760 | @gwenshap >> Follow us: Twitter <https://twitter.com/ConfluentInc> | blog >> <http://www.confluent.io/blog> >> >
