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. 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 > > > > > >
