On Thu, Feb 2, 2017, at 21:45, Becket Qin wrote: > Hi Colin, > > Thanks for the KIP. An admin client is probably a must after we block > direct access to ZK. Some comments and thoughts below: > > 1. Do we have a clear scope for the admin client? It might be worth > thinking about the entire user experience of the admin client. Ideally we > may want to have a single client to do all the administrative work > instead > of having multiple ways to do different things. For example, do we want > to > add topic configurations change API in the admin client? What about > partition movements and preferred leader election? Those are also > administrative tasks which seem reasonable to be integrated into the > admin > client.
Thanks for the comments, Becket! I agree that topic configuration change should be in the administrative client. I have not thought about partition movement or preferred leader election. It probably makes sense to put them in the client as well, but we should probably have a longer discussion about those features later when someone is ready to implement them ;) best, Colin > > 2. Regarding the Future based async Ops v.s. batching of Ops, I would > prefer supporting batching if possible. That usually introduce much less > overhead when doing some big operations, e.g. in controller we have been > putting quite some efforts to batch the requests. For admin client, my > understanding is that the operations are: > a. rare and potentially big > b. likely OK to block (it would be good to see some use cases where a > nonblocking behavior is desired) > c. the efficiency of the operation matters. > Given the above three requirements, it seems a batching blocking API is > fine? > > Thanks, > > Jiangjie (Becket) Qin > > > > On Thu, Feb 2, 2017 at 5:54 PM, Dong Lin <lindon...@gmail.com> wrote: > > > Hey Colin, > > > > Thanks for the KIP. I have a few comments below: > > > > - I share similar view with Ismael that a Future-based API is better. > > PurgeDataFrom() is an example API that user may want to do it > > asynchronously even though there is only one request in flight at a time. > > In the future we may also have some admin operation that allows user to > > move replica from one broker to another, which also needs to work in both > > sync and async style. It seems more generic to return Future<?> for any API > > that requires both mode. > > > > - I am not sure if it is the cleanest way to have enum classes > > CreateTopicsFlags and DeleteTopicsFlags. Are we going to create such class > > for every future API that requires configuration? It may be more generic to > > provide Map<String, ?> to any admin API that operates on multiple entries. > > For example, deleteTopic(Map<String, Boolean>). And it can be Map<String, > > Properties> for those API that requires multiple configs per entry. And we > > can provide default value, doc, config name for those API as we do > > with AbstractConfig. > > > > - I not sure if "Try" is very intuitive to Java developer. Is there any > > counterpart of scala's "Try" in java? We actually have quite a few existing > > classes in Kafka that address the same problem, such as > > ProduceRequestResult, LogAppendResult etc. Maybe we can follow the same > > conversion and use *Result as this class name. > > > > - How are we going to allow user to specify timeout for blocking APIs such > > as deleteTopic? Is this configured per AdminClient, or should it be > > specified in the API parameter? > > > > - Are we going to have this class initiate its own thread, as we do with > > Sender class, to send/receive requests? If yes, it will be useful to have > > have a class that extends AbstractConfig and specifies config and their > > default values. > > > > Thanks, > > Dong > > > > > > > > On Thu, Feb 2, 2017 at 3:02 PM, Ismael Juma <ism...@juma.me.uk> wrote: > > > > > Hi Colin, > > > > > > Thanks for the KIP, great to make progress on this. I have some initial > > > comments, will post more later: > > > > > > 1. We have KafkaProducer that implements the Producer interface and > > > KafkaConsumer that implements the Consumer interface. Maybe we could have > > > KafkaAdminClient that implements the AdminClient interface? Or maybe just > > > KafkaAdmin. Not sure, some ideas for consideration. Also, I don't think > > we > > > should worry about a name clash with the internal AdminClient written in > > > Scala. That will go away soon enough and choosing a good name for the > > > public class is what matters. > > > > > > 2. We should include the proposed package name in the KIP > > > (probably org.apache.kafka.clients.admin?). > > > > > > 3. It would be good to list the supported configs. > > > > > > 4. KIP-107, which passed the vote, specifies the introduction of a method > > > to AdminClient with the following signature. We should figure out how it > > > would look given this proposal. > > > > > > Future<Map<TopicPartition, PurgeDataResult>> > > > purgeDataBefore(Map<TopicPartition, Long> offsetForPartition) > > > > > > 5. I am not sure about rejecting the Futures-based API. I think I would > > > prefer that, personally. Grant had an interesting idea of not exposing > > the > > > batch methods in the AdminClient to start with to keep it simple and > > > relying on a Future based API to make it easy for users to run things > > > concurrently. This is consistent with the producer and Java 8 makes > > things > > > a lot nicer with CompletableFuture (similar to Scala Futures). I will > > think > > > more about this and other details of the proposal and send a follow-up. > > > > > > Ismael > > > > > > On Thu, Feb 2, 2017 at 6:54 PM, Colin McCabe <cmcc...@apache.org> wrote: > > > > > > > Hi all, > > > > > > > > I wrote up a Kafka improvement proposal for adding an > > > > AdministrativeClient interface. This is a continuation of the work on > > > > KIP-4 towards centralized administrative operations. Please check out > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-117% > > > 3A+Add+a+public+ > > > > AdministrativeClient+API+for+Kafka+admin+operations > > > > > > > > regards, > > > > Colin > > > > > > > > >
