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