Hi Matthias, Tests will also create ConsumerRecords. I don't see the point in forcing a bunch of trivial changes to tests, that's all. Of course, it's not too hard to tweak the tests, but it's making work.
Thanks, Andrew ________________________________________ From: Matthias J. Sax <mj...@apache.org> Sent: 27 September 2024 18:36 To: dev@kafka.apache.org <dev@kafka.apache.org> Subject: Re: [DISCUSS] KIP-1094 Add a new constructor method with nextOffsets to ConsumerRecords Thanks for the KIP Alieh. Deprecating the existing constructor won't break anybody atm, right? We don't remove the constructor yet. Of course, in 5.0 in the future we would apply this breaking change. While `ConsumerRecord` is a public API, it seems that actual user code should never create a `ConsumerRecord` object? In general (of course this is not in scope of this KIP) I am wondering why we have a public constructor to begin with (I assume it's some Java limitations how visibility is managed). Having said this, I would also tend to deprecate the old constructor. @Andrew: Why do you prefer to keep the old constructor? To me it seems to be an potential source of bugs, to keep the old one? -Matthias On 9/27/24 2:19 AM, Andrew Schofield wrote: > Hi Alieh, > Thanks for the KIP. Looks like a nice addition to me. I prefer not > deprecating the existing > constructor too. > > Thanks, > Andrew > > ________________________________________ > From: Alieh Saeedi <asae...@confluent.io.INVALID> > Sent: 27 September 2024 09:09 > To: dev@kafka.apache.org <dev@kafka.apache.org> > Subject: Re: [DISCUSS] KIP-1094 Add a new constructor method with nextOffsets > to ConsumerRecords > > Thank you, Bill and Sophie. > > @Bill: You are right. I updated the KIP. > @Sophie: Yes, it was also our concern, but we thought we should keep the > current constructor in order to not break the user's code who has called > this constructor in their code. > > Thanks, > Alieh > > On Fri, Sep 27, 2024 at 1:07 AM Sophie Blee-Goldman <sop...@responsive.dev> > wrote: > >> Should we deprecate the old constructor to make sure that all info gets >> passed in when creating a ConsumerRecords instance? >> >> On Thu, Sep 26, 2024 at 3:37 PM Bill Bejeck <bbej...@apache.org> wrote: >> >>> Hi Alieh, >>> >>> Thanks for the KIP, it will be very useful to Kafka Streams. >>> I have one comment. In the "Proposed Changes" section, you mention the >>> "The `nextOffsets` object contains the next offset and the last leader >>> epoch per partition". >>> If understand the KIP correctly, it should be something along the lines >> of >>> "The `nextOffsets` method returns a map of `TopicPartition` to >>> `OffsetAndMetadata` objects and `OffsetAndMetadata` contains the next >>> offset and the last leader epoch per partition" >>> >>> Other than that, the KIP LGTM. >>> >>> Thanks, >>> Bill >>> >>> >>> On Wed, Sep 25, 2024 at 7:43 AM Alieh Saeedi >> <asae...@confluent.io.invalid >>>> >>> wrote: >>> >>>> Hi all, >>>> >>>> I would like to open a discussion for KIP-1094: Add a new constructor >>>> method with `nextOffsets` to `ConsumerRecords`. >>>> >>>> You can find the detailed proposal here: >>>> >>>> >>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1094%3A+Add+a+new+constructor+method+with+nextOffsets+to+ConsumerRecords >>>> >>>> I look forward to your feedback and suggestions. >>>> >>>> Thanks, >>>> Alieh >>>> >>> >>
