Sophie, yes, that a fair summary, and yes, it was only an alternative
idea for the case that people think, allowing to disable leave-group
request for the plain consumer is not desirable. Seems we are actually
on the same page.
On 9/28/24 2:56 PM, Sophie Blee-Goldman wrote:
@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
