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 > > > >
