Thanks, Sophie and Kirk. You raised good points, Kirk. Regarding your first question: KAFKA-17622 <https://issues.apache.org/jira/browse/KAFKA-17622> (one of the linked jira tickets) points to one reported bug <https://forum.confluent.io/t/kafka-streams-timeout-during-partition-rebalance-seeking-insights-on-notleaderorfollowerexception/11362>. We assume that, it happens due to race conditions. About your second concern: thanks for raising that. I actually overlooked that comment, but I assume that in such a situation, our desired leader epoch is the one from the new `fetch` object. In other words, the `fetch.add(Fetch<K, V> fetch)` should update/add the leader epochs by the ones from the input `fetch` argument. Since the leader epoch associated with the `currentRecords` is somehow the old/former leader epoch. Of course, I am not an expert here. Happy to hear back from experts.
Thanks, Alieh On Tue, Oct 1, 2024 at 12:16 AM Sophie Blee-Goldman <sop...@responsive.dev> wrote: > @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 > > > > >>>> > > > > >>> > > > > >> > > > > > > > > > >
