Hi Ahmed, Thanks for the KIP! It will be a useful addition to know the last-stable-offset (LSO) for a partition. Motivation section is not clear to me. Can you document the scenarios on how the exposed LSO can be used for txn support/debug?
-- Kamal On Thu, Mar 21, 2024 at 11:30 PM Justine Olshan <jols...@confluent.io.invalid> wrote: > 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 > > > > > > >
