On 2023/02/25 08:26:28 David Jacot wrote:
> Hi Chia-Ping,
>
> Thanks for the KIP.
>
> I find the configure method in the proposed interface a bit weird for a few
> reasons. First, it has a default implementation which suggests that it is
> optional but it is not because the InputStream is required. Second, it
oh, my bad. I forgot to remove the default impl after adding the input stream
to config method.
>
> Did we consider using two methods instead of one? We could have configure
> coming from Configurable et setInputStream to set the InputStream. Another
> option would be to have a method which takes the input stream and returns
> an iterator to consume the records.
I prefer to set input stream only once. Also, if Configurable interface is
required for all plugins in kafka code base, the option.2 is suitable - we can
change the returned type of `readRecords(InputStream)` from single record to an
iterator of records. Thus, the new interface not only extends Configurable but
also take input stream only once.
>
> Cheers,
> David
>
> Le mer. 22 févr. 2023 à 11:53, Chia-Ping Tsai <chia7...@apache.org> a
> écrit :
>
> >
> >
> > On 2023/02/22 10:01:29 Alexandre Dupriez wrote:
> > > Hi Chia-Ping,
> > >
> > > Thanks for your answer. Apologies I should have been clearer in the
> > > previous message. What I meant is, is there a plan to use the SPI more
> > > broadly inside the Kafka codebase?
> >
> > no, there is no plan to reuse the SPI.
> >
> > > The question arises because the interface exposes a close() method
> > > which is never invoked by the ConsoleProducer. Hence, although we need
> > > to keep this method to maintain compatibility of the SPI with its
> > > current implementations
> >
> > yep, you are right. the close() method is never executed by the
> > ConsoleProducer. The ConsolerConsumer has similar issue (
> > https://github.com/apache/kafka/pull/8978). I will fix it.
> >
> > > we should perhaps clarify that this method is
> > > not used/deprecated, unless it is intended to be used in the future.
> >
> > I prefer to keep the close() since the new interface is similar to
> > Deserializer. The close() method can be used to notify/release something
> > when the console is going to be down.
> >
> >
>
