Hi David > > - 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 for kind reminder. I have updated the KIP. Please take a look. Thanks > David Jacot <dja...@confluent.io.INVALID> 於 2023年2月27日 下午8:26 寫道： > > 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. >>>> >>>> >>> >>
