Hi lsmael I will update the KIP to replace “ org.apache.kafka.clients.tool” by “ org.apache.kafka.tools” due to following reasons.
1) it is more consistent with existent server/clients pluggable interface package 2) kafka tool jar is not expected to be imported to write code for pluggable interfaces. 3) no new module is introduced. The client module already has pluggable interface of tool, and thus putting them together can make users write other plugins with single (client) jar — Chia-Ping > Ismael Juma <ism...@juma.me.uk> 於 2023年3月15日 上午3:06 寫道： > > Hi Chia, > > Regarding `org.apache.kafka.clients.tool`, a few comments: > > 1. Why is it in `clients`? We don't generally consider tools to be a client. > 2. Why is it `tool`? We have a package `org.apache.kafka.tools`, so it's a > bit odd that this one uses singular instead of plural. > 3. Also, should we follow the `storage-api` example and have a module for > all extensible interfaces used by tools? > > Ismael > >> On Tue, Mar 14, 2023 at 9:23 AM Chia-Ping Tsai <chia7...@gmail.com> wrote: >> >> >> >>>> Chris Egerton <chr...@aiven.io.INVALID> 於 2023年3月15日 上午12:04 寫道： >>> >>> Hi Chia-Ping, >>> >>> Thanks for the KIP. I find the interface definition really polished and >>> intuitive! One small question--I noticed the change of the package to >>> "org.apache.kafka.clients.tool". It doesn't look like there's any >> precedent >>> for using that package. We also use the "org.apache.kafka.common" package >>> for the "MessageFormatter" interface, which is in some ways the >> equivalent >>> pluggable interface for the console consumer. >> >> It seems to me those pluggable interfaces (MessageFormatter and >> RecordReader) should not be a part of “common” package. They are used by >> specify tools only. `Configurable`, by contrast, is good to be located at >> `common` package since it is used widely in our code base. >> >> >>> >>> Do we know if it's necessary to preserve the Checkstyle import >> limitations >>> (which I'm assuming are what motivated the shift in package name)? It >> seems >>> like it might be better to just relax that constraint in order to >> colocate >>> the pluggable interfaces for our console producer/consumer. >> >> I love checkstyle import, and that is one of reason the KIP isolates the >> new interface to a separate package. We have to add `allowed rule` one by >> one if those dedicated interfaces are using the `common` package. The >> constraint of new package can be relax to colocate the pluggable interfaces >> (used by tools), and the `relax` won’t impact other existent packages. >> >> >>> >>> Cheers, >>> >>> Chris >>> >>> On Tue, Mar 7, 2023 at 6:30 AM Chia-Ping Tsai <chia7...@gmail.com> >> wrote: >>> >>>> hi Mickael >>>> >>>>> ?> configs) in the Compatibility, Deprecation, and Migration Plan, I >>>>> guess these can be removed now. >>>> >>>> Done! thanks for feedback >>>> >>>>> Mickael Maison <mickael.mai...@gmail.com> 於 2023年3月7日 下午7:13 寫道： >>>>> >>>>> Hi Chia-Ping, >>>>> >>>>> The new API looks good. >>>>> I still see mentions to configure(InputStream inputStream, Map<String, >>>>> ?> configs) in the Compatibility, Deprecation, and Migration Plan, I >>>>> guess these can be removed now. >>>>> >>>>> Thanks, >>>>> Mickael >>>>> >>>>> On Fri, Mar 3, 2023 at 2:37 PM Chia-Ping Tsai <chia7...@apache.org> >>>> wrote: >>>>>> >>>>>> Dear all, >>>>>> >>>>>> there are some changes for KIP-614 >>>>>> >>>>>> >>>> >> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866569 >>>>>> >>>>>> 1) the interface RecordReader extends Configurable. >>>>>> 2) the input stream is removed from RecordReader#configure method >>>>>> 3) RecordReader#readRecords accept InputStream as argument, and the >>>> returned type is changed from single ProducerRecord to >>>> Iterator<ProducerRecord> >>>>>> >>>>>> Please take a look and then start to vote if you have free time. >> thanks. >>>>>> >>>>>> vote: >> https://lists.apache.org/thread/kjdtyfg5xytn60q0qvxhfopzmfp9tsxr >>>> >>>> >> >>
