Thanks for the update. - The new interface looks good to me. Note that the javadoc does not reflect the new interface though. - Could we precise how errors will be handled? For instance, say that the iterator could translate the input stream to a record. Would calling the next method on the iterator throw an exception?
Thanks, David On Sat, Feb 25, 2023 at 10:43 PM Chia-Ping Tsai <chia7...@apache.org> wrote: > > > 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. > > > > > > > > >
