@Matthias I suspect that last email was meant for KIP-1092? But glad we are on the same page :P
Seems we're on the same page here as well. Not sure if there are any other open questions, otherwise we can maybe move to a vote? On Mon, Sep 30, 2024 at 5:11 AM Alieh Saeedi <asae...@confluent.io.invalid> wrote: > Hi all, > > thanks for the feedbacks. > Since there is consensus in deprecating the current constructor, we can > deprecate it for now and remove it later on (in 5.0 maybe). > The KIP is updated. > > Thanks, > Alieh > > On Sun, Sep 29, 2024 at 6:22 AM Andrew Schofield < > andrew_schofield_j...@outlook.com> wrote: > > > @sophie I meant AK tests. But you make a fair point. There will not be > > that many instances I'm sure, so if the consensus is to deprecate the > > old constructor, that's fine with me. > > > > ________________________________________ > > From: Sophie Blee-Goldman <sop...@responsive.dev> > > Sent: 28 September 2024 22:56 > > To: dev@kafka.apache.org <dev@kafka.apache.org> > > Subject: Re: [DISCUSS] KIP-1094 Add a new constructor method with > > nextOffsets to ConsumerRecords > > > > @Andrew do you mean user tests might create ConsumerRecords, or are you > > concerned about AK tests? > > > > I guess Alieh would be the one to implement this, so I'll leave it up to > > her to investigate the current usage of the existing constructor and > > whether it feels like too much trivial work to remove it from our own AK > > tests. Otherwise, on pure API design philosophy grounds, I would prefer > to > > deprecate and remove the old one. > > > > However if we feel that users have reason to create ConsumerRecords for > > tests then it's probably not worth the upheaval to deprecate the old > > constructor. > > > > On Fri, Sep 27, 2024 at 2:38 PM Andrew Schofield < > > andrew_schofield_j...@outlook.com> wrote: > > > > > 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 > > > >>>> > > > >>> > > > >> > > > > > >
