LGTM! Thanks for raising this improvement. Luke
On Thu, May 23, 2024 at 12:52 AM Chia-Ping Tsai <chia7...@gmail.com> wrote: > Thanks for Josep's response > > > We can add this to 3.8.0, but keep in mind the KIP is not voted yet (as > far > as I can see), so I would highly encourage to start the vote thread ASAP > and strat with the implementation right after. > > sure. We will file a draft PR at the same time! > > Josep Prat <josep.p...@aiven.io.invalid> 於 2024年5月23日 週四 上午12:31寫道： > > > Hi all, > > > > We can add this to 3.8.0, but keep in mind the KIP is not voted yet (as > far > > as I can see), so I would highly encourage to start the vote thread ASAP > > and strat with the implementation right after. > > > > Best, > > > > ----------------- > > Josep Prat > > Open Source Engineering Director, aivenjosep.p...@aiven.io | > > +491715557497 | aiven.io > > Aiven Deutschland GmbH > > Alexanderufer 3-7, 10117 Berlin > > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > > Amtsgericht Charlottenburg, HRB 209739 B > > > > On Wed, May 22, 2024, 17:06 Chia-Ping Tsai <chia7...@gmail.com> wrote: > > > > > > One issue I also noted is that some of the existing Decoder > > > implementations (StringDecoder for example) can accept configurations > > > but currently DumpLogSegments does not provide a way to pass any > > > configurations, it creates an empty VerifiableProperties object each > > > time it instantiates a Decoder instance. If we were to use > > > Deserializer we would also need a way to provide configurations. > > > > > > BTW, if the known bug gets fixed, we have to make new interface extend > > > `configurable`. > > > > > > Or we can just ignore the known issue as `DumpLogSegments` has no > options > > > to take custom configs for `Decoder`. That allow the `Decoder` more > > simple > > > > > > > > > Chia-Ping Tsai <chia7...@gmail.com> 於 2024年5月22日 週三 下午10:58寫道： > > > > > > > > > > > Thanks for Mickael response! > > > > > > > > >I'm wondering whether we need to introduce a new Decoder interface > and > > > > instead if we could reuse Deserializer. We could deprecate the > > > > key-decoder-class and value-decoder-class flags and introduce new > > > > flags like key-deserializer-class and value-deserializer-class. One > > > > benefit is that we already have many existing deserializer > > > > implementations. WDYT? > > > > > > > > I prefer to use different interface, since using the same interface > > > > (Deserializer) may obstruct us from enhancing the interface used by > > > > DumpLogSegments only in the future. > > > > > > > > > One issue I also noted is that some of the existing Decoder > > > > implementations (StringDecoder for example) can accept configurations > > > > but currently DumpLogSegments does not provide a way to pass any > > > > configurations, it creates an empty VerifiableProperties object each > > > > time it instantiates a Decoder instance. If we were to use > > > > Deserializer we would also need a way to provide configurations. > > > > > > > > yep, that is a known issue: > > > > https://issues.apache.org/jira/browse/KAFKA-12311 > > > > > > > > We will file PR to fix it > > > > > > > > Mickael Maison <mickael.mai...@gmail.com> 於 2024年5月22日 週三 下午10:51寫道： > > > > > > > >> Hi, > > > >> > > > >> Thanks for the KIP. Sorting this out in 3.8.0 would be really nice > as > > > >> it would allow us to migrate this tool in 4.0.0. We're unfortunately > > > >> past the KIP deadline but maybe this is small enough to have an > > > >> exception. > > > >> > > > >> I'm wondering whether we need to introduce a new Decoder interface > and > > > >> instead if we could reuse Deserializer. We could deprecate the > > > >> key-decoder-class and value-decoder-class flags and introduce new > > > >> flags like key-deserializer-class and value-deserializer-class. One > > > >> benefit is that we already have many existing deserializer > > > >> implementations. WDYT? > > > >> > > > >> One issue I also noted is that some of the existing Decoder > > > >> implementations (StringDecoder for example) can accept > configurations > > > >> but currently DumpLogSegments does not provide a way to pass any > > > >> configurations, it creates an empty VerifiableProperties object each > > > >> time it instantiates a Decoder instance. If we were to use > > > >> Deserializer we would also need a way to provide configurations. > > > >> > > > >> Thanks, > > > >> Mickael > > > >> > > > >> On Wed, May 22, 2024 at 4:12 PM Chia-Ping Tsai <chia7...@apache.org > > > > > >> wrote: > > > >> > > > > >> > Dear all, > > > >> > > > > >> > We know that 3.8.0 KIP is already frozen, but this is a small KIP > > and > > > >> we need to ship it to 3.8.0 so as to remove the deprecated scala > > > interface > > > >> from 4.0. > > > >> > > > > >> > Best, > > > >> > Chia-Ping > > > >> > > > > >> > On 2024/05/22 14:05:16 Frank Yang wrote: > > > >> > > Hi team, > > > >> > > > > > >> > > Chia-Ping Tsai and I would like to propose KIP-1047 to migrate > > > >> kafka.serializer.Decoder from core module (scala) to tools module > > > (java). > > > >> > > > > > >> > > Feedback and comments are welcome. > > > >> > > > > > >> > > KIP-1047: > > > >> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1047+Introduce+new+org.apache.kafka.tools.api.Decoder+to+replace+kafka.serializer.Decoder > > > >> > > JIRA: https://issues.apache.org/jira/browse/KAFKA-16796 > > > >> > > > > > >> > > Thank you. > > > >> > > PoAn > > > >> > > > > > > > > > >
