Hi all Regarding the second point that Kirk raised, we should always follow the "later write wins" policy (as he stated and confirmed). BTW, we can discuss such details in the PR, I assume.
Since everything is clear and there is no further discussion, we can move to the voting phase. Thanks, Alieh On Tue, Oct 1, 2024 at 1:40 PM Alieh Saeedi <asae...@confluent.io> wrote: > 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 >> > > > >>>> >> > > > >>> >> > > > >> >> > > > >> > > >> > >> >
