I minor comment about `.compacted()` method: It might be better to change to `cleanupPolicy(CleanupPolicy)` and add an enum `CleanupPolicy` with fields `DELETE`, `COMPACT`, `DELETE_COMPACT`.
Also, for the cleanup policy, what about retention time vs size? I am also wondering, what the cut-off line for important configs is, that get their own method, vs all other that are set via `config()`. It seems to be an "random" selection atm. -Matthias On 5/9/19 6:56 PM, Almog Gavra wrote: > Moving discussion on VOTE thread here: > > Ismael asked: > >> 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. > > 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 and 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 Fri, May 3, 2019 at 11:43 AM Randall Hauch <rha...@gmail.com> wrote: > >> I personally like those extra methods, rather than relying upon the generic >> properties. But I'm fine if others think they should be removed. I'm also >> fine with not deprecating the Connect version of the builder. >> >> On Fri, May 3, 2019 at 11:27 AM Almog Gavra <al...@confluent.io> wrote: >> >>> Ack. KIP updated :) Perhaps instead of deprecating the Connect builder, >>> then, we can indeed just subclass it and move some of the less common >> build >>> methods (e.g. uncleanLeaderElection) there. >>> >>> On Fri, May 3, 2019 at 11:20 AM Randall Hauch <rha...@gmail.com> wrote: >>> >>>> Thanks for updating, Almog. I have a few suggestions specific to the >>>> builder: >>>> >>>> 1. The AK pattern for builder classes that are nested is to name the >>> class >>>> "Builder" and to make it publicly visible. We should follow that >> pattern >>>> here, too. >>>> 2. The builder's private constructor makes it impossible to subclass, >>>> should we ever want to do that (e.g, in Connect). If we make it >> protected >>>> or public, then subclassing is easier. >>>> >>>> On Thu, May 2, 2019 at 9:44 AM Almog Gavra <al...@confluent.io> wrote: >>>> >>>>> Thanks for the input Randall. I'm happy adding it natively to >> NewTopic >>>>> instead of introducing more verbosity - updating the KIP to reflect >>> this >>>>> now. >>>>> >>>>> On Thu, May 2, 2019 at 9:28 AM Randall Hauch <rha...@gmail.com> >> wrote: >>>>> >>>>>> 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 >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
signature.asc
Description: OpenPGP digital signature
