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