Hey Ahmed, Echoing Andrew's comments to clean up the public interfaces, it was unclear to me if this new flag applies to all the options or just the "latest" option. Clarifying that and showing a few examples could help.
Thanks, Justine On Thu, Mar 21, 2024 at 9:43 AM Andrew Schofield < andrew_schofield_j...@outlook.com> wrote: > Hi, > Glad to hear you’re better. > > Re-reading the KIP, I think the changes to the public interfaces are > probably only the addition > of the new `--isolation` flag on the `kafka-get-offsets.sh` tool. The > `KafkaAdminClient.listOffsets` > parameters already have what you need I think (OffsetSpec.LATEST and > ListOffsetOptions.isolationLevel). > > If you tidy up the `Public Interfaces` section with the revised syntax for > the tool, I think the > committers will be able to see more easily what the changes are and it > would then be ready to vote > in my opinion. > > Thanks, > Andrew > > > On 20 Mar 2024, at 12:56, Ahmed Sobeh <ahmed.so...@aiven.io.INVALID> > wrote: > > > > Hi Andrew, > > > > Apologies for disappearing, I had to undergo knee surgery, all good now! > > > > I adjusted the KIP as you suggested, do you think it's ready for the > voting > > stage? > > > > On Wed, Mar 6, 2024 at 4:02 PM Andrew Schofield < > > andrew_schofield_j...@outlook.com> wrote: > > > >> 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 > >> > >> > >> > > > > -- > > [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 > > >
