Thanks Colin. It seems like you replied to me accidentally instead of the
list, so leaving your reply below for the benefit of others.
Regarding the disadvantage of having to hunt through the request class,
don't people have to do that anyway with the Options classes?
Aside from that, it would be great if the KIP included more detailed
javadoc for each method including information about potential exceptions.
I'm particularly interested in what a user can expect if a create topics
succeeds versus what they can expect if a timeout exception is thrown. It
would be good to consider partial failures as well.
Ismael
On Fri, Mar 3, 2017 at 9:37 PM, Colin McCabe <cmcc...@apache.org> wrote:
> On Fri, Mar 3, 2017, at 06:41, Ismael Juma wrote:
> > Hi Colin,
> >
> > I still need to do a detailed review, but I have a couple of
> > comments/questions:
> >
> > 1. I am not sure about having the options/response classes as inner
> > classes
> > of the interface. It means that file containing the interface will be
> > huge
> > eventually. And the classes are not necessarily related either. Why not
> > use
> > a separate package for them?
>
> Yeah, I think it's reasonable to make these top-level classes and put
> them in separate files. We can put them all in
> org.apache.kafka.clients.admin.
>
> >
> > 2. Can you elaborate on how one decides one goes in the Options class
> > versus the first parameter?
>
> I guess I think of options as things that you don't have to set. For
> example, when deleting a topic, you must supply the topic name, but
> supplying a non-default timeout is optional.
>
> > I wonder if it would be simpler to just have a
> > single parameter. In that case it should probably be called a Request as
> > Radai suggested, but that's a separate point and we can discuss it
> > separately.
>
> Hmm. I don't think it would be simpler for users. It would force
> people who just want to do something simple like delete a topic or get
> the api version of a single node to go hunting through the request
> class.
>
> best,
> Colin
>
>
> >
> > Ismael
> >
> > On Thu, Mar 2, 2017 at 1:58 AM, Colin McCabe <cmcc...@apache.org> wrote:
> >
> > > On Wed, Mar 1, 2017, at 15:52, radai wrote:
> > > > quick comment on the request objects:
> > > >
> > > > i see "abstract class NewTopic" and "class NewTopicWithReplication"
> and "
> > > > NewTopicWithReplicaAssignments"
> > > >
> > > > 1. since the result object is called CreateTopicResults should these
> be
> > > > called *Request?
> > >
> > > Hi radai,
> > >
> > > Thanks for taking a look.
> > >
> > > I think using the name "request" would be very confusing here, because
> > > we have a whole family of internal Request classes such as
> > > CreateTopicsRequest, TopicMetataRequest, etc. which are used for RPCs.
> > >
> > > > 2. this seems like a suboptimal approach to me. imagine we add a
> > > > NewTopicWithSecurity, and then we would need
> > > > NewTopicWithReplicationAndSecurity? (or any composable "traits").
> > > > this wont really scale. Wouldnt it be better to have a single (rather
> > > complicated)
> > > > CreateTopicRequest, and use a builder pattern to deal with the
> compexity
> > > > and options? like so:
> > > >
> > > > CreateTopicRequest req =
> > > > AdminRequests.newTopic("bob").replicationFactor(2).
> > > withPartitionAssignment(1,
> > > > "boker7", "broker10").withOption(...).build();
> > > >
> > > > the builder would validate any potentially conflicting options and
> would
> > > > allow piling on the complexity in a more manageable way (note - my
> code
> > > > above intends to demonstrate both a general replication factor and a
> > > > specific assignment for a partiocular partition of that topic, which
> may
> > > > be
> > > > too much freedom).
> > >
> > > We don't need to express every optional bell and whistle by creating a
> > > subclass. In fact, the proposal already had setConfigs() in the base
> > > class, since it applies to every new topic creation.
> > >
> > > Thinking about it a little more, though, the subclasses don't really
> add
> > > that much value, so we should probably just have NewTopic and no
> > > subclasses. I removed the subclasses.
> > >
> > > best,
> > > Colin
> > >
> > > >
> > > > On Wed, Mar 1, 2017 at 1:58 PM, Colin McCabe <cmcc...@apache.org>
> wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > Thanks for commenting, everyone. Does anyone have more questions
> or
> > > > > comments, or should we vote? The latest proposal is up at
> > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-
> > > 117%3A+Add+a+public+
> > > > > AdminClient+API+for+Kafka+admin+operations
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Thu, Feb 16, 2017, at 15:00, Colin McCabe wrote:
> > > > > > On Thu, Feb 16, 2017, at 14:11, Dong Lin wrote:
> > > > > > > Hey Colin,
> > > > > > >
> > > > > > > Thanks for the update. I have two comments:
> > > > > > >
> > > > > > > - I actually think it is simpler and good enough to have
> per-topic
> > > API
> > > > > > > instead of batch-of-topic API. This is different from the
> argument
> > > for
> > > > > > > batch-of-partition API because, unlike operation on topic,
> people
> > > > > usually
> > > > > > > operate on multiple partitions (e.g. seek(), purge()) at a
> time. Is
> > > > > there
> > > > > > > performance concern with per-topic API? I am wondering if we
> > > should do
> > > > > > > per-topic API until there is use-case or performance benefits
> of
> > > > > > > batch-of-topic API.
> > > > > >
> > > > > > Yes, there is a performance concern with only supporting
> operations
> > > on
> > > > > > one topic at a time. Jay expressed this in some of his earlier
> > > emails
> > > > > > and some other people did as well. We have cases in mind for
> > > management
> > > > > > software where many topics are created at once.
> > > > > >
> > > > > > >
> > > > > > > - Currently we have interface "Consumer" and "Producer". And we
> > > also
> > > > > have
> > > > > > > implementations of these two interfaces as "KafkaConsumer" and
> > > > > > > "KafkaProducer". If we follow the same naming pattern, should
> we
> > > have
> > > > > > > interface "AdminClient" and the implementation
> "KafkaAdminClient",
> > > > > > > instead
> > > > > > > of the other way around?
> > > > > >
> > > > > > That's a good point. We should do that for consistency.
> > > > > >
> > > > > > best,
> > > > > > Colin
> > > > > >
> > > > > > >
> > > > > > > Dong
> > > > > > >
> > > > > > >
> > > > > > > On Thu, Feb 16, 2017 at 10:19 AM, Colin McCabe <
> cmcc...@apache.org
> > > >
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > So I think people have made some very good points so far.
> There
> > > > > seems
> > > > > > > > to be agreement that we need to have explicit batch APIs for
> the
> > > > > sake of
> > > > > > > > efficiency, so I added that back.
> > > > > > > >
> > > > > > > > Contexts seem a little more complex than we thought, so I
> removed
> > > > > that
> > > > > > > > from the proposal.
> > > > > > > >
> > > > > > > > I removed the Impl class. Instead, we now have a
> > > KafkaAdminClient
> > > > > > > > interface and an AdminClient implementation. I think this
> > > matches
> > > > > our
> > > > > > > > other code better, as Jay commented.
> > > > > > > >
> > > > > > > > Each call now has an "Options" object that is passed in.
> This
> > > will
> > > > > > > > allow us to easily add new parameters to the calls without
> having
> > > > > tons
> > > > > > > > of function overloads. Similarly, each call now has a
> Results
> > > > > object,
> > > > > > > > which will let us easily extend the results we are returning
> if
> > > > > needed.
> > > > > > > >
> > > > > > > > Many people made the point that Java 7 Futures are not that
> > > useful,
> > > > > but
> > > > > > > > Java 8 CompletableFutures are. With CompletableFutures, you
> can
> > > > > chain
> > > > > > > > calls, adapt them, join them-- basically all the stuff
> people are
> > > > > doing
> > > > > > > > in Node.js and Twisted Python. Java 7 Futures don't really
> let
> > > you
> > > > > do
> > > > > > > > anything but poll for a value or block. So I felt that it
> was
> > > > > better to
> > > > > > > > just go with a CompletableFuture-based API.
> > > > > > > >
> > > > > > > > People also made the point that they would like an easy API
> for
> > > > > waiting
> > > > > > > > on complete success of a batch call. For example, an API
> that
> > > would
> > > > > > > > fail if even one topic wasn't created in createTopics. So I
> > > came up
> > > > > > > > with Result objects that provide multiple futures that you
> can
> > > wait
> > > > > on.
> > > > > > > > You can wait on a future that fires when everything is
> complete,
> > > or
> > > > > you
> > > > > > > > can wait on futures for individual topics being created.
> > > > > > > >
> > > > > > > > I updated the wiki, so please take a look. Note that this
> new
> > > API
> > > > > > > > requires JDK8. It seems like JDK8 is coming soon, though,
> and
> > > the
> > > > > > > > disadvantages of sticking to Java 7 are pretty big here, I
> think.
> > > > > > > >
> > > > > > > > best,
> > > > > > > > Colin
> > > > > > > >
> > > > > > > >
> > > > > > > > On Mon, Feb 13, 2017, at 11:51, Colin McCabe wrote:
> > > > > > > > > On Sun, Feb 12, 2017, at 09:21, Jay Kreps wrote:
> > > > > > > > > > Hey Colin,
> > > > > > > > > >
> > > > > > > > > > Thanks for the hard work on this. I know going back and
> > > forth on
> > > > > APIs
> > > > > > > > is
> > > > > > > > > > kind of frustrating but we're at the point where these
> things
> > > > > live long
> > > > > > > > > > enough and are used by enough people that it is worth the
> > > pain.
> > > > > I'm
> > > > > > > > sure
> > > > > > > > > > it'll come down in the right place eventually. A couple
> > > things
> > > > > I've
> > > > > > > > found
> > > > > > > > > > helped in the past:
> > > > > > > > > >
> > > > > > > > > > 1. The burden of evidence needs to fall on the
> > > complicator.
> > > > > i.e. if
> > > > > > > > > > person X thinks the api should be async they need to
> > > produce
> > > > > a set
> > > > > > > > of
> > > > > > > > > > common use cases that require this. Otherwise you are
> > > > > perpetually
> > > > > > > > > > having to
> > > > > > > > > > think "we might need x". I think it is good to have a
> > > rule of
> > > > > > > > "simple
> > > > > > > > > > until
> > > > > > > > > > proven insufficient".
> > > > > > > > > > 2. Make sure we frame things for the intended
> audience. At
> > > > > this
> > > > > > > > point
> > > > > > > > > > our apis get used by a very broad set of Java
> engineers.
> > > This
> > > > > is a
> > > > > > > > > > very
> > > > > > > > > > different audience from our developer mailing list.
> These
> > > > > people
> > > > > > > > code
> > > > > > > > > > for a
> > > > > > > > > > living not necessarily as a passion, and may not
> > > understand
> > > > > details
> > > > > > > > of
> > > > > > > > > > the
> > > > > > > > > > internals of our system or even basic things like
> > > > > multi-threaded
> > > > > > > > > > programming. I don't think this means we want to dumb
> > > things
> > > > > down,
> > > > > > > > but
> > > > > > > > > > rather try really hard to make things truly simple
> when
> > > > > possible.
> > > > > > > > > >
> > > > > > > > > > Okay here were a couple of comments:
> > > > > > > > > >
> > > > > > > > > > 1. Conceptually what is a TopicContext? I think it
> means
> > > > > something
> > > > > > > > > > like
> > > > > > > > > > TopicAdmin? It is not literally context about Topics
> > > right?
> > > > > What is
> > > > > > > > > > the
> > > > > > > > > > relationship of Contexts to clients? Is there a
> > > threadsafety
> > > > > > > > > > difference?
> > > > > > > > > > Would be nice to not have to think about this, this is
> > > what I
> > > > > mean
> > > > > > > > by
> > > > > > > > > > "conceptual weight". We introduce a new concept that
> is a
> > > bit
> > > > > > > > nebulous
> > > > > > > > > > that
> > > > > > > > > > I have to figure out to use what could be a simple
> api.
> > > I'm
> > > > > sure
> > > > > > > > > > you've
> > > > > > > > > > been through this experience before where you have
> these
> > > > > various
> > > > > > > > > > objects
> > > > > > > > > > and you're trying to figure out what they represent
> (the
> > > > > connection
> > > > > > > > to
> > > > > > > > > > the
> > > > > > > > > > server? the information to create a connection? a
> request
> > > > > session?).
> > > > > > > > >
> > > > > > > > > The intention was to provide some grouping of methods, and
> > > also a
> > > > > place
> > > > > > > > > to put request parameters which were often set to defaults
> > > rather
> > > > > than
> > > > > > > > > being explicitly set. If it seems complex, we can
> certainly
> > > get
> > > > > rid of
> > > > > > > > > it.
> > > > > > > > >
> > > > > > > > > > 2. We've tried to avoid the Impl naming convention. In
> > > > > general the
> > > > > > > > > > rule
> > > > > > > > > > has been if there is only going to be one
> implementation
> > > you
> > > > > don't
> > > > > > > > > > need an
> > > > > > > > > > interface. If there will be multiple, distinguish it
> from
> > > the
> > > > > > > > others.
> > > > > > > > > > The
> > > > > > > > > > other clients follow this pattern: Producer,
> > > KafkaProducer,
> > > > > > > > > > MockProducer;
> > > > > > > > > > Consumer, KafkaConsumer, MockConsumer.
> > > > > > > > >
> > > > > > > > > Good point. Let's change the interface to
> KafkaAdminClient,
> > > and
> > > > > the
> > > > > > > > > implementation to AdminClient.
> > > > > > > > >
> > > > > > > > > > 3. We generally don't use setters or getters as a
> naming
> > > > > > > > convention. I
> > > > > > > > > > personally think mutating the setting in place seems
> kind
> > > of
> > > > > like
> > > > > > > > late
> > > > > > > > > > 90s
> > > > > > > > > > Java style. I think it likely has thread-safety
> issues.
> > > i.e.
> > > > > even if
> > > > > > > > > > it is
> > > > > > > > > > volatile you may not get the value you just set if
> there
> > > is
> > > > > another
> > > > > > > > > > thread... I actually really liked what you described
> as
> > > your
> > > > > > > > original
> > > > > > > > > > idea
> > > > > > > > > > of having a single parameter object like
> > > CreateTopicRequest
> > > > > that
> > > > > > > > holds
> > > > > > > > > > all
> > > > > > > > > > these parameters and defaults. This lets you evolve
> the
> > > api
> > > > > with all
> > > > > > > > > > the
> > > > > > > > > > various combinations of arguments without overloading
> > > > > insanity.
> > > > > > > > After
> > > > > > > > > > doing
> > > > > > > > > > literally tens of thousands of remote APIs at
> LinkedIn we
> > > > > eventually
> > > > > > > > > > converged on a rule, which is ultimately every remote
> api
> > > > > needs a
> > > > > > > > > > single
> > > > > > > > > > argument object you can add to over time and it must
> be
> > > > > batched.
> > > > > > > > Which
> > > > > > > > > > brings me to my next point...
> > > > > > > > >
> > > > > > > > > Just to clarify, volatiles were never a part of the
> proposal.
> > > I
> > > > > think
> > > > > > > > > that context objects or request objects should be used by a
> > > single
> > > > > > > > > thread at a time.
> > > > > > > > >
> > > > > > > > > I'm not opposed to request objects, but I think they raise
> all
> > > the
> > > > > same
> > > > > > > > > questions as context objects. Basically, the thread-safety
> > > issues
> > > > > need
> > > > > > > > > to be spelled out and understood by the user, and the user
> > > needs
> > > > > more
> > > > > > > > > lines of code to make a request. And there will be people
> > > trying
> > > > > to do
> > > > > > > > > things like re-use request objects when they should not,
> and so
> > > > > forth.
> > > > > > > > >
> > > > > > > > > > 4. I agree batch apis are annoying but I suspect we'll
> > > end up
> > > > > adding
> > > > > > > > > > one. Doing 1000 requests for 1000 operations if
> creating
> > > or
> > > > > deleting
> > > > > > > > > > will
> > > > > > > > > > be bad, right? This won't be the common case, but when
> > > you do
> > > > > it it
> > > > > > > > > > will be
> > > > > > > > > > a deal-breaker problem. I don't think we should try
> to fix
> > > > > this one
> > > > > > > > > > behind
> > > > > > > > > > the scenes.
> > > > > > > > > > 5. Are we going to do CompletableFuture (which
> requires
> > > java
> > > > > 8) or
> > > > > > > > > > normal Future? Normal Future is utterly useless for
> most
> > > > > things
> > > > > > > > other
> > > > > > > > > > than
> > > > > > > > > > just calling wait. If we can evolve in place from
> Future
> > > to
> > > > > > > > > > CompletableFuture that is fantastic (we could do it
> for
> > > the
> > > > > producer
> > > > > > > > > > too!).
> > > > > > > > > > My belief was that this was binary incompatible but I
> > > > > actually don't
> > > > > > > > > > know
> > > > > > > > > > (obviously it's source compatible).
> > > > > > > > >
> > > > > > > > > In my testing, replacing a return type with a subclass of
> that
> > > > > return
> > > > > > > > > type did not break binary compatibility. I haven't been
> able
> > > to
> > > > > find
> > > > > > > > > chapter and verse on this from the Java implementers,
> though.
> > > > > > > > >
> > > > > > > > > best,
> > > > > > > > > Colin
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > -Jay
> > > > > > > > > >
> > > > > > > > > > On Wed, Feb 8, 2017 at 5:00 PM, Colin McCabe <
> > > cmcc...@apache.org
> > > > > >
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi all,
> > > > > > > > > > >
> > > > > > > > > > > I made some major revisions to the proposal on the
> wiki, so
> > > > > please
> > > > > > > > check
> > > > > > > > > > > it out.
> > > > > > > > > > >
> > > > > > > > > > > The new API is based on Ismael's suggestion of grouping
> > > > > related APIs.
> > > > > > > > > > > There is only one layer of grouping. I think that it's
> > > > > actually
> > > > > > > > pretty
> > > > > > > > > > > intuitive. It's also based on the idea of using
> Futures,
> > > which
> > > > > > > > several
> > > > > > > > > > > people commented that they'd like to see.
> > > > > > > > > > >
> > > > > > > > > > > Here's a simple example:
> > > > > > > > > > >
> > > > > > > > > > > > AdminClient client = new AdminClientImpl(myConfig);
> > > > > > > > > > > > try {
> > > > > > > > > > > > client.topics().create("foo", 3, (short) 2,
> > > false).get();
> > > > > > > > > > > > Collection<String> topicNames =
> > > > > client.topics().list(false).
> > > > > > > > get();
> > > > > > > > > > > > log.info("Found topics: {}",
> > > Utils.mkString(topicNames,
> > > > > ", "));
> > > > > > > > > > > > Collection<Node> nodes =
> client.nodes().list().get();
> > > > > > > > > > > > log.info("Found cluster nodes: {}",
> > > > > Utils.mkString(nodes, ",
> > > > > > > > "));
> > > > > > > > > > > > } finally {
> > > > > > > > > > > > client.close();
> > > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > The good thing is, there is no Try, no 'get' prefixes,
> no
> > > > > messing
> > > > > > > > with
> > > > > > > > > > > batch APIs. If there is an error, then Future#get()
> > > throws an
> > > > > > > > > > > ExecutionException which wraps the relevant exception
> in
> > > the
> > > > > standard
> > > > > > > > > > > Java way.
> > > > > > > > > > >
> > > > > > > > > > > Here's a slightly less simple example:
> > > > > > > > > > >
> > > > > > > > > > > > AdminClient client = new AdminClientImpl(myConfig);
> > > > > > > > > > > > try {
> > > > > > > > > > > > List<Future<Void>> futures = new LinkedList<>();
> > > > > > > > > > > > for (String topicName: myNewTopicNames) {
> > > > > > > > > > > > creations.add(client.topics().
> > > > > > > > > > > > setClientTimeout(30000).setCreationConfig(
> > > > > myTopicConfig).
> > > > > > > > > > > > create(topicName, 3, (short) 2, false));
> > > > > > > > > > > > }
> > > > > > > > > > > > Futures.waitForAll(futures);
> > > > > > > > > > > > } finally {
> > > > > > > > > > > > client.close();
> > > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > I went with Futures because I feel like ought to have
> some
> > > > > option for
> > > > > > > > > > > doing async. It's a style of programming that has
> become a
> > > > > lot more
> > > > > > > > > > > popular with the rise of Node.js, Twisted python, etc.
> etc.
> > > > > Also, as
> > > > > > > > > > > Ismael commented, Java 8 CompletableFuture is going to
> make
> > > > > Java's
> > > > > > > > > > > support for fluent async programming a lot stronger by
> > > > > allowing call
> > > > > > > > > > > chaining and much more.
> > > > > > > > > > >
> > > > > > > > > > > If we are going to support async, the simplest thing is
> > > just
> > > > > to make
> > > > > > > > > > > everything return a future and let people call get() if
> > > they
> > > > > want to
> > > > > > > > run
> > > > > > > > > > > synchronously. Having a mix of async and sync APIs is
> just
> > > > > going to
> > > > > > > > be
> > > > > > > > > > > confusing and redundant.
> > > > > > > > > > >
> > > > > > > > > > > I think we should try to avoid creating single
> functions
> > > that
> > > > > start
> > > > > > > > > > > multiple requests if we can. It makes things much
> > > uglier. It
> > > > > means
> > > > > > > > > > > that you have to have some kind of request class that
> > > wraps up
> > > > > the
> > > > > > > > > > > request the user is trying to create, so that you can
> > > handle an
> > > > > > > > array of
> > > > > > > > > > > those requests. The return value has to be something
> like
> > > > > Map<Node,
> > > > > > > > > > > Try<Value>> to represent which nodes failed and
> succeeded.
> > > > > This is
> > > > > > > > the
> > > > > > > > > > > kind of stuff that, in my opinion, makes people scratch
> > > their
> > > > > heads.
> > > > > > > > > > >
> > > > > > > > > > > If we need to, we can still get some of the efficiency
> > > > > benefits of
> > > > > > > > batch
> > > > > > > > > > > APIs by waiting for a millisecond or two before sending
> > > out a
> > > > > topic
> > > > > > > > > > > create() request to see if other create() requests
> > > arrive. If
> > > > > so, we
> > > > > > > > > > > can coalesce them. It might be better to figure out if
> > > this
> > > > > is an
> > > > > > > > > > > actual performance issue before implementing it,
> though.
> > > > > > > > > > >
> > > > > > > > > > > I think it would be good to get something out there,
> > > annotate
> > > > > it as
> > > > > > > > > > > @Unstable, and get feedback from people building
> against
> > > trunk
> > > > > and
> > > > > > > > using
> > > > > > > > > > > it. We have removed or changed @Unstable APIs in
> streams
> > > > > before, so
> > > > > > > > I
> > > > > > > > > > > don't think we should worry that it will get set in
> stone
> > > > > > > > prematurely.
> > > > > > > > > > > The AdminClient API should get much less developer use
> than
> > > > > anything
> > > > > > > > in
> > > > > > > > > > > streams, so changing an unstable API should be much
> easier.
> > > > > > > > > > >
> > > > > > > > > > > best,
> > > > > > > > > > > Colin
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Wed, Feb 8, 2017, at 07:49, Ismael Juma wrote:
> > > > > > > > > > > > Thanks for elaborating Jay. I totally agree that we
> have
> > > to
> > > > > be very
> > > > > > > > > > > > careful
> > > > > > > > > > > > in how we use our complexity budget. Easier said than
> > > done
> > > > > when
> > > > > > > > people
> > > > > > > > > > > > don't agree on what is complex and what is simple.
> :) For
> > > > > example,
> > > > > > > > I
> > > > > > > > > > > > think
> > > > > > > > > > > > batch APIs are a significant source of complexity as
> you
> > > > > have to
> > > > > > > > do a
> > > > > > > > > > > > bunch
> > > > > > > > > > > > of ceremony to group things before sending the
> request
> > > and
> > > > > error
> > > > > > > > handling
> > > > > > > > > > > > becomes more complex due to partial failures (things
> like
> > > > > `Try` or
> > > > > > > > other
> > > > > > > > > > > > mechanisms that serve a similar role are then
> needed).
> > > > > > > > > > > >
> > > > > > > > > > > > Maybe a way forward is to write API usage examples to
> > > help
> > > > > > > > validate that
> > > > > > > > > > > > the suggested API is indeed easy to use.
> > > > > > > > > > > >
> > > > > > > > > > > > Ismael
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, Feb 8, 2017 at 4:40 AM, Jay Kreps <
> > > j...@confluent.io>
> > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > Totally agree on CompletableFuture. Also agree with
> > > some
> > > > > of the
> > > > > > > > rough
> > > > > > > > > > > edges
> > > > > > > > > > > > > on the Consumer.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I don't have much of a leg to stand on with the
> > > splitting
> > > > > vs not
> > > > > > > > > > > splitting
> > > > > > > > > > > > > thing, really hard to argue one or the other is
> > > better. I
> > > > > guess
> > > > > > > > the one
> > > > > > > > > > > > > observation in watching us try to make good public
> apis
> > > > > over the
> > > > > > > > years
> > > > > > > > > > > is I
> > > > > > > > > > > > > am kind of in favor of a particular kind of
> simple. In
> > > > > > > > particular I
> > > > > > > > > > > think
> > > > > > > > > > > > > since the bar is sooo high in support and docs and
> the
> > > > > community
> > > > > > > > of
> > > > > > > > > > > users
> > > > > > > > > > > > > so broad in the range of their capabilities, it
> makes
> > > it so
> > > > > > > > there is a
> > > > > > > > > > > lot
> > > > > > > > > > > > > of value in dead simple interfaces that don't have
> a
> > > lot of
> > > > > > > > conceptual
> > > > > > > > > > > > > weight, don't introduce a lot of new classes or
> > > concepts or
> > > > > > > > general
> > > > > > > > > > > > > patterns that must be understood to use them
> > > correctly. So
> > > > > > > > things like
> > > > > > > > > > > > > nesting, or the Try class, or async apis, or even
> just
> > > a
> > > > > complex
> > > > > > > > set of
> > > > > > > > > > > > > classes representing arguments or return values
> kind of
> > > > > all stack
> > > > > > > > > > > > > conceptual burdens on the user to figure out
> correct
> > > > > usage. So
> > > > > > > > like,
> > > > > > > > > > > for
> > > > > > > > > > > > > example, the Try class is very elegant and
> represents a
> > > > > whole
> > > > > > > > > > > generalized
> > > > > > > > > > > > > class of possibly completed actions, but the flip
> side
> > > is
> > > > > maybe
> > > > > > > > I'm
> > > > > > > > > > > just a
> > > > > > > > > > > > > working guy who needs to list his kafka topics but
> > > doesn't
> > > > > know
> > > > > > > > Rust,
> > > > > > > > > > > take
> > > > > > > > > > > > > pity on me! :-)
> > > > > > > > > > > > >
> > > > > > > > > > > > > Nit picking aside, super excited to see us
> progress on
> > > > > this.
> > > > > > > > > > > > >
> > > > > > > > > > > > > -Jay
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Tue, Feb 7, 2017 at 3:46 PM Ismael Juma <
> > > > > ism...@juma.me.uk>
> > > > > > > > wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Jay,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks for the feedback. Comments inline.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Tue, Feb 7, 2017 at 8:18 PM, Jay Kreps <
> > > > > j...@confluent.io>
> > > > > > > > wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - I think it would be good to not use "get"
> as
> > > the
> > > > > prefix
> > > > > > > > for
> > > > > > > > > > > things
> > > > > > > > > > > > > > > making remote calls. We've tried to avoid
> the
> > > java
> > > > > getter
> > > > > > > > > > > convention
> > > > > > > > > > > > > > > entirely (see code style guide), but for
> remote
> > > > > calls in
> > > > > > > > > > > particular
> > > > > > > > > > > > > it
> > > > > > > > > > > > > > > kind
> > > > > > > > > > > > > > > of blurs the line between field access and
> > > remote
> > > > > RPC in
> > > > > > > > a way
> > > > > > > > > > > that
> > > > > > > > > > > > > > > leads
> > > > > > > > > > > > > > > people to trouble. What about, e.g.,
> > > > > fetchAllGroups() vs
> > > > > > > > > > > > > > getAllGroups().
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Agreed that we should avoid the `get` prefix for
> > > remote
> > > > > calls.
> > > > > > > > There
> > > > > > > > > > > are
> > > > > > > > > > > > > a
> > > > > > > > > > > > > > few possible prefixes for the read operations:
> list,
> > > > > fetch,
> > > > > > > > describe.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - I think futures and callbacks are a bit
> of a
> > > pain
> > > > > to
> > > > > > > > use. I'd
> > > > > > > > > > > > > second
> > > > > > > > > > > > > > > Becket's comment: let's ensure there a
> common
> > > use
> > > > > case
> > > > > > > > > > > motivating
> > > > > > > > > > > > > > these
> > > > > > > > > > > > > > > that wouldn't be just as easily satisfied
> with
> > > batch
> > > > > > > > operations
> > > > > > > > > > > > > (which
> > > > > > > > > > > > > > > we
> > > > > > > > > > > > > > > seem to have at least for some things). In
> > > terms of
> > > > > > > > flexibility
> > > > > > > > > > > > > > > Callbacks >
> > > > > > > > > > > > > > > Futures > Batch Ops but I think in terms of
> > > > > usability it
> > > > > > > > is the
> > > > > > > > > > > > > exact
> > > > > > > > > > > > > > > opposite so let's make sure we have worked
> out
> > > how
> > > > > the
> > > > > > > > API will
> > > > > > > > > > > be
> > > > > > > > > > > > > > used
> > > > > > > > > > > > > > > before deciding. In particular I think java
> > > Futures
> > > > > are
> > > > > > > > often an
> > > > > > > > > > > > > > > uncomfortable half-way point since calling
> > > get() and
> > > > > > > > blocking
> > > > > > > > > > > the
> > > > > > > > > > > > > > > thread is
> > > > > > > > > > > > > > > often not what you want for chaining
> sequences
> > > of
> > > > > > > > operations in
> > > > > > > > > > > a
> > > > > > > > > > > > > > truly
> > > > > > > > > > > > > > > async way, so 99% of people just use the
> future
> > > as
> > > > > a way
> > > > > > > > to
> > > > > > > > > > > batch
> > > > > > > > > > > > > > calls.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > We should definitely figure out how the APIs are
> > > going
> > > > > to be
> > > > > > > > used
> > > > > > > > > > > before
> > > > > > > > > > > > > > deciding. I agree that callbacks are definitely
> > > painful
> > > > > and
> > > > > > > > there's
> > > > > > > > > > > > > little
> > > > > > > > > > > > > > reason to expose them in a modern API unless it's
> > > meant
> > > > > to be
> > > > > > > > very
> > > > > > > > > > > low
> > > > > > > > > > > > > > level. When it comes to Futures, I think it's
> > > important
> > > > > to
> > > > > > > > > > > distinguish
> > > > > > > > > > > > > what
> > > > > > > > > > > > > > is available in Java 7 and below versus what is
> > > > > available from
> > > > > > > > Java 8
> > > > > > > > > > > > > > onwards. CompletableFuture makes it much easier
> to
> > > > > > > > compose/chain
> > > > > > > > > > > > > operations
> > > > > > > > > > > > > > (in a similar vein to java.util.Stream, our own
> > > Streams
> > > > > API,
> > > > > > > > etc.)
> > > > > > > > > > > and it
> > > > > > > > > > > > > > gives you the ability to register callbacks if
> you
> > > > > really want
> > > > > > > > to
> > > > > > > > > > > > > (avoiding
> > > > > > > > > > > > > > the somewhat odd situation in the Producer where
> we
> > > > > return a
> > > > > > > > Future
> > > > > > > > > > > _and_
> > > > > > > > > > > > > > allow you to pass a callback).
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > - Personally I don't think splitting the
> admin
> > > > > methods up
> > > > > > > > > > > actually
> > > > > > > > > > > > > > makes
> > > > > > > > > > > > > > > things more usable. It just makes you have
> to
> > > dig
> > > > > through
> > > > > > > > our
> > > > > > > > > > > > > > > hierarchy. I
> > > > > > > > > > > > > > > think a flat class with a bunch of
> operations
> > > (like
> > > > > the
> > > > > > > > consumer
> > > > > > > > > > > > > api)
> > > > > > > > > > > > > > is
> > > > > > > > > > > > > > > probably the easiest for people to grok and
> find
> > > > > things
> > > > > > > > on. I am
> > > > > > > > > > > > > kind
> > > > > > > > > > > > > > > of a
> > > > > > > > > > > > > > > dumb PHP programmer at heart, though.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I am not sure it's fair to compare the
> AdminClient
> > > with
> > > > > the
> > > > > > > > > > > Consumer. The
> > > > > > > > > > > > > > former has APIs for a bunch of unrelated APIs
> > > (topics,
> > > > > ACLs,
> > > > > > > > configs,
> > > > > > > > > > > > > > consumer groups, delegation tokens, preferred
> leader
> > > > > election,
> > > > > > > > > > > partition
> > > > > > > > > > > > > > reassignment, etc.) where the latter is pretty
> > > > > specialised.
> > > > > > > > For each
> > > > > > > > > > > of
> > > > > > > > > > > > > the
> > > > > > > > > > > > > > resources, you may have 3-4 operations, it will
> get
> > > > > confusing
> > > > > > > > fast.
> > > > > > > > > > > Also,
> > > > > > > > > > > > > > do you really think an API that has one level of
> > > > > grouping will
> > > > > > > > mean
> > > > > > > > > > > that
> > > > > > > > > > > > > > users have to "dig through our hierarchy"? Or
> are you
> > > > > > > > concerned that
> > > > > > > > > > > once
> > > > > > > > > > > > > > we go in that direction, there is a danger of
> making
> > > the
> > > > > > > > hierarchy
> > > > > > > > > > > more
> > > > > > > > > > > > > > complicated?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Finally, I am not sure I would use the consumer
> as an
> > > > > example
> > > > > > > > of
> > > > > > > > > > > > > something
> > > > > > > > > > > > > > that is easy to grok. :) The fact that methods
> behave
> > > > > pretty
> > > > > > > > > > > differently
> > > > > > > > > > > > > > (some are blocking while others only have an
> effect
> > > after
> > > > > > > > poll) with
> > > > > > > > > > > no
> > > > > > > > > > > > > > indication from the type signature or naming
> > > convention
> > > > > makes
> > > > > > > > it
> > > > > > > > > > > harder,
> > > > > > > > > > > > > > not easier, to understand.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Ismael
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > >
> > > > >
> > >
>
