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
