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