Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks Matthias. 1. PR looks good to me. 2. About adding default methods, I think for correctness and backward compatibility default methods should be added to avoid breaking potential clients. I have updated the KIP accordingly. Jorge. On Thu, Jul 23, 2020 at 5:19 AM Matthias J. Sax wrote:

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Finally cycling back to this. Overall I like the KIP. Two comments: - I tried to figure out why the two InMemoerySessionStore methods are deprecated and it seems those annotations are there since the class was added; as this seems to be a bug, and there are no backward compatibility concerns,

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks Jorge, This KIP looks good to me! -John On Fri, Jul 3, 2020, at 03:19, Jorge Esteban Quilcate Otoya wrote: > Hi John, > > Thanks for the feedback. > > I'd be happy to take the third option and consider moving methods to > ReadOnlySessionStore as part of the KIP. > Docs is updated to

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi John, Thanks for the feedback. I'd be happy to take the third option and consider moving methods to ReadOnlySessionStore as part of the KIP. Docs is updated to reflect these changes. Cheers, Jorge. On Thu, Jul 2, 2020 at 10:06 PM John Roesler wrote: > Hey Jorge, > > Thanks for the

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hey Jorge, Thanks for the details. That sounds like a mistake to me on both counts. I don’t think you need to worry about those depreciations. If the interface methods aren’t deprecated, then the methods are not deprecated. We should remove the annotations, but it doesn’t need to be in the

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

(sorry for the spam) Actually `findSessions` are only deprecated on `InMemorySessionStore`, which seems strange as RocksDB and interfaces haven't marked these methods as deprecated. Any hint on how to handle this? Cheers, On Thu, Jul 2, 2020 at 4:57 PM Jorge Esteban Quilcate Otoya <

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

@John: I can see there are some deprecations in there as well. Will update the KIP. Thanks, Jorge On Thu, Jul 2, 2020 at 3:29 PM Jorge Esteban Quilcate Otoya < quilcate.jo...@gmail.com> wrote: > Thanks John. > > > it looks like there’s a revision error in which two methods are proposed > for

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks John. > it looks like there’s a revision error in which two methods are proposed for SessionStore, but seem like they should be in ReadOnlySessionStore. Do I read that right? Yes, I've opted to keep the new methods alongside the existing ones. In the case of SessionStore, `findSessions`

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi Jorge, Thanks for the update. I think this is a good plan. I just took a look at the KIP again, and it looks like there’s a revision error in which two methods are proposed for SessionStore, but seem like they should be in ReadOnlySessionStore. Do I read that right? Otherwise, I’m happy

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Quick update: KIP is updated with latest changes now. Will leave it open this week while working on the PR. Hope to open a new vote thread over the next few days if no additional feedback is provided. Cheers, Jorge. On Mon, Jun 29, 2020 at 11:30 AM Jorge Esteban Quilcate Otoya <

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks, John! Make sense to reconsider the current approach. I was heading in a similar direction while drafting the implementation. Metered, Caching, and other layers will also have to get duplicated to build up new methods in `Stores` factory, and class casting issues would appear on stores

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi Jorge, Sorry for my silence, I've been absorbed with the 2.6 and 2.5.1 releases. The idea to separate the new methods into "mixin" interfaces seems like a good one, but as we've discovered in KIP-614, it doesn't work out that way in practice. The problem is that the store implementations are

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi everyone, I've updated the KIP, applying Matthias' feedback regarding interface hierarchy. Also, following the last email, I think we can consider reverse operations on KeyValue range as well, as implementation supports lexicographic order. I considered different naming between Key-based

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi everyone, sorry for the late reply. Thanks Matthias for your feedback. I think it makes sense to reconsider the current design based on your input. After digging deeper into the current implementation, I'd like to bring my current understanding to be double-checked as it might be redefining

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hey, Sorry that I am late to the game. I am not 100% convinced about the current proposal. Using a new config as feature flag seems to be rather "nasty" to me, and flipping from/to is a little bit too fancy for my personal taste. I agree, that the original proposal using a "ReadDirection" enum

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Sorry for my silence, Jorge, I've just taken another look, and I'm personally happy with the KIP. Thanks, -John On Tue, May 26, 2020, at 16:17, Jorge Esteban Quilcate Otoya wrote: > If no additional comments, I will proceed to start the a vote thread. > > Thanks a lot for your feedback! > >

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

If no additional comments, I will proceed to start the a vote thread. Thanks a lot for your feedback! On Fri, May 22, 2020 at 9:25 AM Jorge Esteban Quilcate Otoya < quilcate.jo...@gmail.com> wrote: > Thanks Sophie. I like the `reverseAll()` idea. > > I updated the KIP with your feedback. > > >

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks Sophie. I like the `reverseAll()` idea. I updated the KIP with your feedback. On Fri, May 22, 2020 at 4:22 AM Sophie Blee-Goldman wrote: > Hm, the case of `all()` does seem to present a dilemma in the case of > variable-length keys. > > In the case of fixed-length keys, you can just

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hm, the case of `all()` does seem to present a dilemma in the case of variable-length keys. In the case of fixed-length keys, you can just compute the keys that correspond to the maximum and minimum serialized bytes, then perform a `range()` query instead of an `all()`. If your keys don't have a

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks John. Agree. I like the first approach as well, with StreamsConfig flag passing by via ProcessorContext. Another positive effect with "reverse parameters" is that in the case of `fetch(keyFrom, keyTo, timeFrom, timeTo)` users can decide _which_ pair to flip, whether with `ReadDirection`

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi Jorge, Thanks for that idea. I agree, a feature flag would protect anyone who may be depending on the current behavior. It seems better to locate the feature flag in the initialization logic of the store, rather than have a method on the "live" store that changes its behavior on the fly. It

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thank you both for the great feedback. I like the "fancy" proposal :), and how it removes the need for additional API methods. And with a feature flag on `StateStore`, disabled by default, should no break current users. The only side-effect I can think of is that: by moving the flag upwards, all

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

> There's no possibility that someone could be relying > on iterating over that range in increasing order, because that's not what > happens. However, they could indeed be relying on getting an empty iterator I just meant that they might be relying on the assumption that the range query will

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks for the response, Sophie, I wholeheartedly agree we should take as much into account as possible up front, rather than regretting our decisions later. I actually do share your vague sense of worry, which was what led me to say initially that I thought my counterproposal might be "too

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

> Rather than working around it, I think we should just fix it Now *that's* a "fancy" idea :P That was my primary concern, although I do have a vague sense of worry that we might be allowing users to get into trouble without realizing it. For example if their custom serdes suffer a similar bug

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Thanks Sophie, Woah, that’s a nasty bug. Rather than working around it, I think we should just fix it. I’ll leave some comments on the Jira. It doesn’t seem like it should be this KIP’s concern that some serdes might be incorrectly written. Were there other practical concerns that you had in

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

I like this "fancy idea" to just flip the to/from bytes but I think there are some practical limitations to implementing this. In particular I'm thinking about this issue with the built-in signed number serdes. This trick would actually fix the

Re: [DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi there Jorge, Thanks for the KIP! I think this feature sounds very reasonable. I'm not 100% sure if this is "too fancy", but what do you think about avoiding the enum by instead allowing people to flip the "from" and "to" endpoints? I.e., reading from "A" to "Z" would be a forward scan, and

[DISCUSS] KIP-617: Allow Kafka Streams State Stores to be iterated backwards

Hi everyone, I would like to start the discussion for KIP-617: https://cwiki.apache.org/confluence/display/KAFKA/KIP-617%3A+Allow+Kafka+Streams+State+Stores+to+be+iterated+backwards Looking forward to your feedback. Thanks! Jorge. 0x5F2C6E22064982DF.asc Description: application/pgp-keys