Hi Matthias. Thanks for the review
1. looks like the majority of us are leaning towards BY_DURATION naming. I have updated the same in the KIP. 2. Thanks. Updated the KIP to remove private/internal implementation. Thanks, On Wed, Nov 13, 2024 at 6:28 AM Matthias J. Sax <mj...@apache.org> wrote: > 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 > >>>>>> > >>>> > >>> > >> > > >
