Ok, I just talked about it with Matthias, I will change the text back to following the code, and update them together.
Sincerely, Hanyu On Tue, Oct 3, 2023 at 3:49 PM Bill Bejeck <bbej...@apache.org> wrote: > Hi Hanyu, > > Thanks for the KIP, overall it's looking good, but I have a couple of > comments > > - In the “Proposed Changes” section there's a reference to > `setReverse()` but the code example has “withDescendingOrder()” so I > think > the text needs an update to reflect the code example. > - I prefer “withDescendingKeys()” to “withDescendingOrder()” > - I also agree that we should include a section on ordering, but it > should be fairly straightforward. The “StateQueryRequest” of IQv2 > allows > users to specify a partition or partitions, so if ordering is important > they can elect to provide a single partition in the query. > > > Thanks, > Bill > > On Tue, Oct 3, 2023 at 5:27 PM Walker Carlson > <wcarl...@confluent.io.invalid> > wrote: > > > Hello Hanyu, > > > > Looking over your kip things mostly make sense but I have a couple of > > comments. > > > > > > 1. You have "withDescandingOrder()". I think you mean "descending" :) > > Also there are still a few places in the do where its called > > "setReverse" > > 2. Also I like "WithDescendingKeys()" better > > 3. I'm not sure of what ordering guarantees we are offering. Perhaps > we > > can add a section to the motivation clearly spelling out the current > > ordering and the new offering? > > 4. When you say "use unbounded reverseQuery to achieve reverseAll" do > > you mean "use unbounded RangeQuery to achieve reverseAll"? as far as I > > can > > tell we don't have a reverseQuery as a named object? > > > > > > Looking good so far > > > > best, > > Walker > > > > On Tue, Oct 3, 2023 at 2:13 PM Colt McNealy <c...@littlehorse.io> wrote: > > > > > Hello Hanyu, > > > > > > Thank you for the KIP. I agree with Matthias' proposal to keep the > naming > > > convention consistent with KIP-969. I favor the `.withDescendingKeys()` > > > name. > > > > > > I am curious about one thing. RocksDB guarantees that records returned > > > during a range scan are lexicographically ordered by the bytes of the > > keys > > > (either ascending or descending order, as specified in the query). This > > > means that results within a single partition are indeed ordered.** My > > > reading of KIP-805 suggests to me that you don't need to specify the > > > partition number you are querying in IQv2, which means that you can > have > > a > > > valid reversed RangeQuery over a store with "multiple partitions" in > it. > > > > > > Currently, IQv1 does not guarantee order of keys in this scenario. Does > > > IQv2 support ordering across partitions? Such an implementation would > > > require opening a rocksdb range scan** on multiple rocksdb instances > (one > > > per partition), and polling the first key of each. Whether or not this > is > > > ordered, could we please add that to the documentation? > > > > > > **(How is this implemented/guaranteed in an `inMemoryKeyValueStore`? I > > > don't know about that implementation). > > > > > > Colt McNealy > > > > > > *Founder, LittleHorse.dev* > > > > > > > > > On Tue, Oct 3, 2023 at 1:35 PM Hanyu (Peter) Zheng > > > <pzh...@confluent.io.invalid> wrote: > > > > > > > ok, I will update it. Thank you Matthias > > > > > > > > Sincerely, > > > > Hanyu > > > > > > > > On Tue, Oct 3, 2023 at 11:23 AM Matthias J. Sax <mj...@apache.org> > > > wrote: > > > > > > > > > Thanks for the KIP Hanyu! > > > > > > > > > > > > > > > I took a quick look and it think the proposal makes sense overall. > > > > > > > > > > A few comments about how to structure the KIP. > > > > > > > > > > As you propose to not add `ReverseRangQuery` class, the code > example > > > > > should go into "Rejected Alternatives" section, not in the > "Proposed > > > > > Changes" section. > > > > > > > > > > For the `RangeQuery` code example, please omit all existing methods > > > etc, > > > > > and only include what will be added/changed. This make it simpler > to > > > > > read the KIP. > > > > > > > > > > > > > > > nit: typo > > > > > > > > > > > the fault value is false > > > > > > > > > > Should be "the default value is false". > > > > > > > > > > > > > > > Not sure if `setReverse()` is the best name. Maybe > > > `withDescandingOrder` > > > > > (or similar, I guess `withReverseOrder` would also work) might be > > > > > better? Would be good to align to KIP-969 proposal that suggest do > > use > > > > > `withDescendingKeys` methods for "reverse key-range"; if we go with > > > > > `withReverseOrder` we should change KIP-969 accordingly. > > > > > > > > > > Curious to hear what others think about naming this consistently > > across > > > > > both KIPs. > > > > > > > > > > > > > > > -Matthias > > > > > > > > > > > > > > > On 10/3/23 9:17 AM, Hanyu (Peter) Zheng wrote: > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-985%3A+Add+reverseRange+and+reverseAll+query+over+kv-store+in+IQv2 > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > [image: Confluent] <https://www.confluent.io> > > > > Hanyu (Peter) Zheng he/him/his > > > > Software Engineer Intern > > > > +1 (213) 431-7193 <+1+(213)+431-7193> > > > > Follow us: [image: Blog] > > > > < > > > > > > > > > > https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog > > > > >[image: > > > > Twitter] <https://twitter.com/ConfluentInc>[image: LinkedIn] > > > > <https://www.linkedin.com/in/hanyu-peter-zheng/>[image: Slack] > > > > <https://slackpass.io/confluentcommunity>[image: YouTube] > > > > <https://youtube.com/confluent> > > > > > > > > [image: Try Confluent Cloud for Free] > > > > < > > > > > > > > > > https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic > > > > > > > > > > > > > > > -- [image: Confluent] <https://www.confluent.io> Hanyu (Peter) Zheng he/him/his Software Engineer Intern +1 (213) 431-7193 <+1+(213)+431-7193> Follow us: [image: Blog] <https://www.confluent.io/blog?utm_source=footer&utm_medium=email&utm_campaign=ch.email-signature_type.community_content.blog>[image: Twitter] <https://twitter.com/ConfluentInc>[image: LinkedIn] <https://www.linkedin.com/in/hanyu-peter-zheng/>[image: Slack] <https://slackpass.io/confluentcommunity>[image: YouTube] <https://youtube.com/confluent> [image: Try Confluent Cloud for Free] <https://www.confluent.io/get-started?utm_campaign=tm.fm-apac_cd.inbound&utm_source=gmail&utm_medium=organic>
