Thanks for updating the KIP.
I am happy to see that we seem to align to use a single config only :)
Obviously, I need to bikeshed on the format: `BACK:<duration>` does not
read well IMHO, and I think `auto.offset.reset="BACK_BY:<duration>"`
would read much better. Andrew's suggestion of `BY_DURATION:<duration>`
might even be better (but I would be ok with either one).
(I would not use `DURATION:<duration>` personally -- similar to
`BACK:<duration>` is does not read well from my POV).
About KS related changes. Overall LGTM. What is the reason for having
AutoOffsetReset.LATEST
AutoOffsetReset.EARLIEST
as `public` variables? Seems we don't need them, but that they are
rather an internal implementation detail?
In general, I would recommend to omit everything `private` on the KIP
(including method implementations), as it's not user facing, but only
have method signatures in the KIP.
What I believe we need to add is a `protected` constructor (for the
internal sub-class):
protected AutoOffsetReset(AutoOffsetReset autoOffsetReset);
-Matthias
On 11/12/24 6:09 AM, Apoorv Mittal wrote:
Thanks Manikumar for explaining, sounds good to me.
Regards,
Apoorv Mittal
On Tue, Nov 12, 2024 at 1:47 PM Andrew Schofield <
andrew_schofield_j...@outlook.com> wrote:
Hi,
Looks good now. Just one suggestion.
AS8: Instead of "back:30D", I wonder whether the word 'duration' ought to
be
used to be consistent with kafka-consumer-groups.sh. So,
"by-duration:P3D" or "duration:P3D" might be better.
The overall idea of merging the configs into one config is fine in the
current
text of the KIP.
Thanks,
Andrew
________________________________________
From: Manikumar <manikumar.re...@gmail.com>
Sent: 12 November 2024 13:30
To: dev@kafka.apache.org <dev@kafka.apache.org>
Subject: Re: [DISCUSS] KIP-1106: Add duration based offset reset option
for consumer clients
Hi Apoorv,
AM7: AutoOffsetReset.java is for Kafka Streams API. I am not proposing any
public Interface/class for Kafka Consumer.
As mentioned in the KIP, even though OffsetResetStrategy is a public class,
it's not used in any public APIs. I think new internal classes should be
sufficient.
AM8: Fixed
AM9: This class is for kafka streams API
AM10: I was using the EARLIEST_TIMESTAMP, LATEST_TIMESTAMP constant values
from ListOffsetsRequest
<
https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/requests/ListOffsetsRequest.java#L41
But yes, we can use Optional in AutoOffsetReset class. Updated the KIP.
Thanks.
On Tue, Nov 12, 2024 at 6:15 PM Apoorv Mittal <apoorvmitta...@gmail.com>
wrote:
Hi,
I read the changes for single configuration and deprecated
OffsetResetStrategy.java.
AM7: Question: The KIP says that previous supported values were
earliest/latest/none and new back:<duration> config would be added. We
have
no definition of "none" in the newly introduced AutoOffsetReset.java
class
hence I am assuming that if "none" is specified as a config option then
that config will be ignored, correct? Or are we deprecating the usage of
"none" altogether?
AM8: Minor: The new class AutoOffsetReset under interfaces mentions the
name as OffsetResetStrategy.java. This requires correction.
AM9: Is the package name correct for AutoOffsetReset as
org.apache.kafka.streams, shouldn't it be under clients package?
AM10: What does -1L and -2L mean as long for latest and earliest in
AutoOffsetReset.java? Is it just a long placeholder which will never be
used elsewhere for latest and earliest? If yes then does it make sense to
keep long as Optional, and use Optional.empty() for latest and earliest?
Regards,
Apoorv Mittal
On Tue, Nov 12, 2024 at 12:06 PM Manikumar <manikumar.re...@gmail.com>
wrote:
Thanks Ismael and Lianet for the reviews.
Based on suggestions, I have updated the KIP to in favour of having a
single config (auto.offset.reset).
I have also adopted the Lianet's suggestion on naming.
auto.offset.reset=back:P3D -> reset back 3 days
Let me know if there are any concerns.
Thanks,
On Sat, Nov 9, 2024 at 10:06 PM Lianet M. <liane...@gmail.com> wrote:
Hi all. Thanks Manikumar for the nice improvement, useful indeed.
I also lean towards having a single config given that it's all about
the
reset policy, seems all the same "what" (auto reset policy) and we
are
just extending the with a new behaviour. Regarding the naming (agree
on
duration being confusing), what about something to show that it's
simply
about how far back to reset:
auto.offset.reset=EARLIEST
auto.offset.reset=BACK:P3D -> reset back 3 days (combined with
ISO8601
seems really easy to read/understand from the config definition
itself)
Something like that would definitely break consistency with the
command
line tool argument "by_duration", but if it seems clearer we should
consider the tradeoff and not penalize the consumer API/configs.
Thanks!
Lianet
On Sat, Nov 9, 2024 at 2:47 AM Ismael Juma <m...@ismaeljuma.com>
wrote:
Thanks for the KIP, this is useful. A comment below.
On Thu, Nov 7, 2024 at 4:51 PM Matthias J. Sax <mj...@apache.org>
wrote:
I am personally not convinced that adding a new config
`auto.offset.reset.by.duration` is the best way. Kafka in
general
has
way too many configs and trying to avoid adding more configs
seems
to be
desirable? -- It seems this might be a point of contention, and
if
the
majority of people wants this new config so be it. I just wanted
to
stress my concerns about it.
I agree that we don't need a new config. We can simply use a value
for
the
existing config. I think a prefix followed by the relevant ISO8601
string
would be clear enough. For example, "by-duration:P23DT23H" or
something
along those lines. I do find the "by-duration" description a bit
confusing
for what we're doing here (i.e. current time - duration) although
there is
precedent in the reset offsets tool.
Ismael
