Hi Ahmed, Looks good to me with one remaining comment. You’ve used:
bin/kafka-get-offsets.sh … --time -1 --isolation -committed bin/kafka-get-offsets.sh … --time latest --isolation -committed bin/kafka-get-offsets.sh … --time -1 --isolation -uncommitted bin/kafka-get-offsets.sh … --time latest --isolation -uncommitted I find the flags starting with a single - to be a bit unusual and doesn’t match the --time options such as “latest”. I suggest: bin/kafka-get-offsets.sh … --time -1 --isolation committed bin/kafka-get-offsets.sh … --time latest --isolation committed bin/kafka-get-offsets.sh … --time -1 --isolation uncommitted bin/kafka-get-offsets.sh … --time latest --isolation uncommitted Or even: bin/kafka-get-offsets.sh … --time -1 --isolation read-committed bin/kafka-get-offsets.sh … --time latest --isolation read-committed bin/kafka-get-offsets.sh … --time -1 --isolation read-uncommitted bin/kafka-get-offsets.sh … --time latest --isolation read-uncommitted Apart from that nit, looks good to me. Thanks, Andrew > On 5 Mar 2024, at 16:35, Ahmed Sobeh <ahmed.so...@aiven.io.INVALID> wrote: > > I adjusted the KIP according to what we agreed on, let me know if you have > any comments! > > Best, > Ahmed > > On Thu, Feb 29, 2024 at 1:44 AM Luke Chen <show...@gmail.com> wrote: > >> Hi Ahmed, >> >> Thanks for the KIP! >> >> Comments: >> 1. If we all agree with the suggestion from Andrew, you could update the >> KIP. >> >> Otherwise, LGTM! >> >> >> Thanks. >> Luke >> >> On Thu, Feb 29, 2024 at 1:32 AM Andrew Schofield < >> andrew_schofield_j...@outlook.com> wrote: >> >>> Hi Ahmed, >>> Could do. Personally, I find the existing “--time -1” totally horrid >>> anyway, which was why >>> I suggested an alternative. I think your suggestion of a flag for >>> isolation level is much >>> better than -6. >>> >>> Maybe I should put in a KIP which adds: >>> --latest (as a synonym for --time -1) >>> --earliest (as a synonym for --time -2) >>> --max-timestamp (as a synonym for --time -3) >>> >>> That’s really what I would prefer. If the user has a timestamp, use >>> `--time`. If they want a >>> specific special offset, use a separate flag. >>> >>> Thanks, >>> Andrew >>> >>>> On 28 Feb 2024, at 09:22, Ahmed Sobeh <ahmed.so...@aiven.io.INVALID> >>> wrote: >>>> >>>> Hi Andrew, >>>> >>>> Thanks for the hint! That sounds reasonable, do you think adding a >>>> conditional argument, something like "--time -1 --isolation -committed" >>> and >>>> "--time -1 --isolation -uncommitted" would make sense to keep the >>>> consistency of getting the offset by time? or do you think having a >>> special >>>> argument for this case is better? >>>> >>>> On Tue, Feb 27, 2024 at 2:19 PM Andrew Schofield < >>>> andrew_schofield_j...@outlook.com> wrote: >>>> >>>>> Hi Ahmed, >>>>> Thanks for the KIP. It looks like a useful addition. >>>>> >>>>> The use of negative timestamps, and in particular letting the user use >>>>> `--time -1` or the equivalent `--time latest` >>>>> is a bit peculiar in this tool already. The negative timestamps come >>> from >>>>> org.apache.kafka.common.requests.ListOffsetsRequest, >>>>> but you’re not actually adding another value to that. As a result, I >>>>> really wouldn’t recommend using -6 for the new >>>>> flag. LSO is really a variant of -1 with read_committed isolation >> level. >>>>> >>>>> I think that perhaps it would be better to add `--last-stable` as an >>>>> alternative to `--time`. Then you’ll get the LSO with >>>>> cleaner syntax. >>>>> >>>>> Thanks, >>>>> Andrew >>>>> >>>>> >>>>>> On 27 Feb 2024, at 10:12, Ahmed Sobeh <ahmed.so...@aiven.io.INVALID> >>>>> wrote: >>>>>> >>>>>> Hi all, >>>>>> I would like to start a discussion on KIP-1021, which would enable >>>>> getting >>>>>> LSO in kafka-get-offsets.sh: >>>>>> >>>>> >>> >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1021%3A+Allow+to+get+last+stable+offset+%28LSO%29+in+kafka-get-offsets.sh >>>>>> >>>>>> Best, >>>>>> Ahmed >>>>> >>>>> >>>> >>>> -- >>>> [image: Aiven] <https://www.aiven.io/> >>>> *Ahmed Sobeh* >>>> Engineering Manager OSPO, *Aiven* >>>> ahmed.so...@aiven.io <i...@aiven.io> >>>> aiven.io <https://www.aiven.io/> | < >>> https://www.facebook.com/aivencloud> >>>> <https://www.linkedin.com/company/aiven/> < >>> https://twitter.com/aiven_io> >>>> *Aiven Deutschland GmbH* >>>> Immanuelkirchstraße 26, 10405 Berlin >>>> Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen >>>> Amtsgericht Charlottenburg, HRB 209739 B >>> >>> >>> >> > > > -- > [image: Aiven] <https://www.aiven.io/> > *Ahmed Sobeh* > Engineering Manager OSPO, *Aiven* > ahmed.so...@aiven.io <i...@aiven.io> > aiven.io <https://www.aiven.io/> | <https://www.facebook.com/aivencloud> > <https://www.linkedin.com/company/aiven/> <https://twitter.com/aiven_io> > *Aiven Deutschland GmbH* > Immanuelkirchstraße 26, 10405 Berlin > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > Amtsgericht Charlottenburg, HRB 209739 B
