On Tue, Mar 14, 2017, at 18:43, Becket Qin wrote:
> Hi Colin,
> Thanks for the reply. Please see comments inline.
>
> On Tue, Mar 14, 2017 at 5:30 PM, Colin McCabe <cmcc...@apache.org> wrote:
> > I'm not sure if there is a benefit to removing createTopic and
> > deleteTopic. It seems like users who want to create or delete just one
> > topic would have to add more boilerplate. I'm curious what others think
> > here.
>
> I think many users actually consume from only one topic. I had the same
> doubt about whether a bathing only API would cause problem for the users
> when we changed the consumer API, but it seem to be fine. For users who
> wants to only operate on a single topic they can do something like:
>
> createTopics(Collcetions.singleton("topic"));
>
> This looks clean enough.
Hi Becket,
I don't feel very strongly about this. But so far I don't see a strong
disadvantage to having both the singular and plural forms. Further
extensions to the API will all be through the CreateTopicsOptions and
NewTopic classes, so the number should stay fixed at 4 rather than
growing.
>
> >
> > >
> > > 2. NewTopic.setConfigs() is a little weird, can it just be part of the
> > > constructor? Any specific reason to change the configs after the creation
> > > of a NewTopic instance?
> >
> > I guess the thinking here was that the constructor contains the fields
> > that are mandatory (name and partition assignments / strategy), but
> > setting a non-default config is optional.
> >
> Can we just have two constructors? I understand that sometimes the
> exploding number of constructors is annoying, but having two constructors
> here seems acceptable. It would avoid introducing the mutability which
> seems not expected.
We already have two constructors, so we would have four if we added
another degree of freedom. But I suppose we can do it, to preserve
immutability. I will change the proposal.
best,
Colin
>
> >
> > cheers,
> > Colin
> >
> >
> > >
> > > Thanks,
> > >
> > > Jiangjie (Becket) Qin
> > >
> > > On Tue, Mar 14, 2017 at 10:15 AM, Ismael Juma <ism...@juma.me.uk> wrote:
> > >
> > > > Hi Gwen,
> > > >
> > > > I agree with you that it's unfortunate that we now have 4 methods per
> > > > operation (including the batch ones). I personally still prefer the
> > option
> > > > of using a single class to represent the parameters of each method
> > (i.e. 1
> > > > batch and 1 non-batch method per operation). I would follow a very
> > simple
> > > > and boring pattern for each operation:
> > > >
> > > > CreateTopicResults createTopic(CreateTopicRequest request);
> > > > CreateTopicsResults createTopics(CreateTopicsRequest request);
> > > >
> > > > This is simple to understand, easy to keep consistent as we add
> > methods and
> > > > easy to evolve. A downside is that it's a bit more verbose than the
> > current
> > > > proposal.
> > > >
> > > > Regarding LogConfig, we can't use it as it's in the core module (and in
> > > > Scala).
> > > >
> > > > I'm not too sure about the Unstable point. A couple of points:
> > > >
> > > > 1. A major reason why we are adding the AdminClient is so that people
> > don't
> > > > have to deal with incompatible changes. So, unless we truly have to, I
> > > > don't think we'll be making incompatible changes after the release.
> > > >
> > > > 2. If we make changes to unstable code, it still has to go through the
> > KIP
> > > > process. If they're minor changes in this release cycle, we can simply
> > send
> > > > an email to the discuss thread, but if it's a major change, then it
> > would
> > > > require a new round of voting (or a new KIP).
> > > >
> > > > Anyway, I think we're close on this one, but it would be good to agree
> > on
> > > > the general pattern so that we can easily add more methods.
> > > >
> > > > Ismael
> > > >
> > > > On Tue, Mar 14, 2017 at 5:29 AM, Gwen Shapira <g...@confluent.io>
> > wrote:
> > > >
> > > > > I'm torn between my desire to get this in already and the fact that
> > parts
> > > > > of the API feel a bit alien to Kafka.
> > > > >
> > > > > I will resolve my difficulties by giving my feedback here and then
> > going
> > > > to
> > > > > vote +1 on the vote thread.
> > > > > Colin can choose whether to address my concerns now or use his
> > "unstable"
> > > > > option to wait and see...
> > > > >
> > > > > My main concern is the RequestOptions objects... It was discussed
> > > > earlier a
> > > > > bit, but I'm not sure my particular concerns were addressed. I see
> > the
> > > > > following issues with it:
> > > > > * They double the number of methods we have
> > > > > * They are pretty similar, so it isn't clear why we need all those
> > > > > different objects
> > > > > * All our other APIs specify timeouts either in method calls
> > directly or
> > > > in
> > > > > the configuration for the entire object.
> > > > >
> > > > > We also typically don't use methods that start with "set", but this
> > is
> > > > > minor.
> > > > >
> > > > > The configs for NewTopic are Map<String, String> - shouldn't we use
> > the
> > > > > LogConfig object that we already have? This will take care of
> > > > documentation
> > > > > and be similar to ProducerConfig and ConsumerConfig?
> > > > >
> > > > > My concerns aside, thank you for working on this much needed API.
> > > > >
> > > > > Gwen
> > > > >
> > > > > On Mon, Mar 13, 2017 at 3:26 PM, Colin McCabe <cmcc...@apache.org>
> > > > wrote:
> > > > >
> > > > > > Hi Radai,
> > > > > >
> > > > > > Thanks for looking at the KIP again.
> > > > > >
> > > > > > On Mon, Mar 13, 2017, at 12:33, radai wrote:
> > > > > > > looking at the KIP as it is now, looks like all *Options objects
> > > > have a
> > > > > > > common timeout property. could it be extracted to a common
> > > > > > > AdminRequestOptions or something?
> > > > > >
> > > > > > Perhaps I'm missing something, but I don't think there is any
> > reason to
> > > > > > extract the timeout property. It doesn't simplify the
> > implementation
> > > > > > (I've already implemented the interface in a branch, so I know
> > this for
> > > > > > sure.) It doesn't simplify the API exposed to the users, since
> > they
> > > > > > will still want to provide the specific option type corresponding
> > to
> > > > the
> > > > > > call. Also, as we discussed previously in the thread (about
> > NewTopic),
> > > > > > having lot of inheritance and base classes makes it difficult to
> > change
> > > > > > classes over time. It is better to simply use composition.
> > > > > >
> > > > > > I think it would be much better to get the AdminClient interface
> > in,
> > > > and
> > > > > > start iterating on it incrementally as we discover ways it could be
> > > > > > better. This is similar to how some things in Streams were added
> > as
> > > > > > unstable interfaces and then stabilized over time.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > On Thu, Mar 9, 2017 at 2:14 PM, Colin McCabe <cmcc...@apache.org
> > >
> > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > We've been discussing this for a while (about a month) and I
> > think
> > > > > > > > people have made some great points that improved the
> > proposal. In
> > > > > > > > particular, adding async and batching was important. I've also
> > > > been
> > > > > > > > talking with some end-users who would like to make use of this
> > API.
> > > > > > > > Once this is in, we can iterate on it before the release, and
> > it
> > > > will
> > > > > > > > also unblock a lot of other admin proposals. I think it would
> > be
> > > > > good
> > > > > > > > to start the vote in a little bit, assuming there are no
> > > > objections.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
