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 reflect these changes.
>
> Cheers,
> Jorge.
>
> On Thu, Jul 2, 2020 at 10:06 PM John Roesler <vvcep...@apache.org> wrote:
>
> > 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 kip.
> >
> > I think any query methods should have been in the ReadOnly interface. I
> > guess it’s up to you whether you want to:
> > 1. Add the reverse methods next to the existing methods (what you have in
> > the kip right now)
> > 2. Partially fix it by adding your new methods to the ReadOnly interface
> > 3. Fully fix the problem by moving the existing methods as well as your
> > new ones. Since SessionStore extends ReadOnlySessionStore, it’s ok just to
> > move the definitions.
> >
> > I’m ok with whatever you prefer.
> >
> > Thanks,
> > John
> >
> > On Thu, Jul 2, 2020, at 11:29, Jorge Esteban Quilcate Otoya wrote:
> > > (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 <
> > > quilcate.jo...@gmail.com> wrote:
> > >
> > > > @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 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` are in `SessionStore`, and
> > `fetch`
> > > >> are in `ReadOnlySessionStore`. If it makes more sense, I can move all
> > of
> > > >> them to ReadOnlySessionStore.
> > > >> Let me know what you think.
> > > >>
> > > >> Thanks,
> > > >> Jorge.
> > > >>
> > > >> On Thu, Jul 2, 2020 at 2:36 PM John Roesler <vvcep...@apache.org>
> > wrote:
> > > >>
> > > >>> 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 with your proposal.
> > > >>>
> > > >>> Thanks,
> > > >>> John
> > > >>>
> > > >>> On Wed, Jul 1, 2020, at 17:01, Jorge Esteban Quilcate Otoya wrote:
> > > >>> > 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 <
> > > >>> > quilcate.jo...@gmail.com> wrote:
> > > >>> >
> > > >>> > > 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 created
> > by
> > > >>> DSL.
> > > >>> > >
> > > >>> > > I will draft a proposal with new methods (move methods from
> > proposed
> > > >>> > > interfaces to existing ones) with default implementation in a KIP
> > > >>> update
> > > >>> > > and wait for Matthias to chime in to validate this approach.
> > > >>> > >
> > > >>> > > Jorge.
> > > >>> > >
> > > >>> > >
> > > >>> > > On Sat, Jun 27, 2020 at 4:01 PM John Roesler <
> > vvcep...@apache.org>
> > > >>> wrote:
> > > >>> > >
> > > >>> > >> 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 just the base layer that get composed with other layers in
> > > >>> Streams
> > > >>> > >> before they can be accessed in the DSL. This is extremely
> > subtle, so
> > > >>> > >> I'm going to put everyone to sleep with a detailed explanation:
> > > >>> > >>
> > > >>> > >> For example, this is the mechanism by which all KeyValueStore
> > > >>> > >> implementations get added to Streams:
> > > >>> > >>
> > org.apache.kafka.streams.state.internals.KeyValueStoreBuilder#build
> > > >>> > >> return new MeteredKeyValueStore<>(
> > > >>> > >> maybeWrapCaching(maybeWrapLogging(storeSupplier.get())),
> > > >>> > >> storeSupplier.metricsScope(),
> > > >>> > >> time,
> > > >>> > >> keySerde,
> > > >>> > >> valueSerde
> > > >>> > >> );
> > > >>> > >>
> > > >>> > >> In the DSL, the store that a processor gets from the context
> > would
> > > >>> be
> > > >>> > >> the result of this composition. So even if the
> > storeSupplier.get()
> > > >>> returns
> > > >>> > >> a store that implements the "reverse" interface, when you try to
> > > >>> use it
> > > >>> > >> from a processor like:
> > > >>> > >> org.apache.kafka.streams.kstream.ValueTransformerWithKey#init
> > > >>> > >> ReadOnlyBackwardWindowStore<K, V> store =
> > > >>> > >> (ReadOnlyBackwardWindowStore<K, V>) context.getStateStore(..)
> > > >>> > >>
> > > >>> > >> You'd just get a ClassCastException because it's actually a
> > > >>> > >> MeteredKeyValueStore, which doesn't implement
> > > >>> > >> ReadOnlyBackwardWindowStore.
> > > >>> > >>
> > > >>> > >> The only way to make this work would be to make the Metered,
> > > >>> > >> Caching, and Logging layers also implement the new interfaces,
> > > >>> > >> but this effectively forces implementations to also implement
> > > >>> > >> the interface. Otherwise, the intermediate layers would have to
> > > >>> > >> cast the store in each method, like this:
> > > >>> > >> MeteredWindowStore#backwardFetch {
> > > >>> > >> ((ReadOnlyBackwardWindowStore<K, V>)
> > innerStore).backwardFetch(..)
> > > >>> > >> }
> > > >>> > >>
> > > >>> > >> And then if the implementation doesn't "opt in" by implementing
> > > >>> > >> the interface, you'd get a ClassCastException, not when you get
> > the
> > > >>> > >> store, but when you try to use it.
> > > >>> > >>
> > > >>> > >> The fact that we get ClassCastExceptions no matter which way we
> > > >>> > >> turn here indicates that we're really not getting any benefit
> > from
> > > >>> the
> > > >>> > >> type system, which makes the extra interfaces seem not worth
> > all the
> > > >>> > >> code involved.
> > > >>> > >>
> > > >>> > >> Where we landed in KIP-614 is that, unless we want to completely
> > > >>> > >> revamp the way that StateStores work in the DSL, you might as
> > > >>> > >> well just add the new methods to the existing interfaces. To
> > prevent
> > > >>> > >> compilation errors, we can add default implementations that
> > throw
> > > >>> > >> UnsupportedOperationException. If a store doesn't opt in by
> > > >>> > >> implementing the methods, you'd get an
> > > >>> UnsupportedOperationException,
> > > >>> > >> which seems no worse, and maybe better, than the
> > ClassCastException
> > > >>> > >> you'd get if we go with the "mixin interface" approach.
> > > >>> > >>
> > > >>> > >> A quick note: This entire discussion focuses on the DSL. If
> > you're
> > > >>> just
> > > >>> > >> using the Processor API by directly adding the a custom store
> > to the
> > > >>> > >> Topology:
> > > >>> > >> org.apache.kafka.streams.Topology#addStateStore
> > > >>> > >> and then retrieving it in the processor via:
> > > >>> > >>
> > org.apache.kafka.streams.processor.ProcessorContext#getStateStore
> > > >>> > >> in
> > > >>> > >> org.apache.kafka.streams.processor.Processor#init
> > > >>> > >>
> > > >>> > >> Then, you can both register and retrieve _any_ StateStore
> > > >>> implementation.
> > > >>> > >> There's no need to use KeyValueStore or any other built-in
> > > >>> interface.
> > > >>> > >> In other words, KeyValueStore and company are only part of the
> > DSL,
> > > >>> > >> not the PAPI. So, discussions about the build-in store
> > interfaces
> > > >>> are only
> > > >>> > >> really relevant in the context of the DSL, Transformers, and
> > > >>> Materialized.
> > > >>> > >>
> > > >>> > >> So, in conclusion, I'd really recommend just adding any new
> > methods
> > > >>> to
> > > >>> > >> the existing store interfaces. We might be able to revamp the
> > API
> > > >>> in the
> > > >>> > >> future to support mixins, but it's a much larger scope of work
> > than
> > > >>> this
> > > >>> > >> KIP.
> > > >>> > >> A more minor comment is that we don't need to add Deprecated
> > > >>> variants
> > > >>> > >> of new methods.
> > > >>> > >>
> > > >>> > >> Thanks again, and once again, I'm sorry I tuned out and didn't
> > > >>> offer this
> > > >>> > >> feedback before you revised the KIP.
> > > >>> > >> -John
> > > >>> > >>
> > > >>> > >>
> > > >>> > >>
> > > >>> > >>
> > > >>> > >> On Mon, Jun 22, 2020, at 06:11, Jorge Esteban Quilcate Otoya
> > wrote:
> > > >>> > >> > 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 ranges and
> > > >>> Time-based
> > > >>> > >> > ranges, and mitigate confusion when fetching keys and time
> > ranges
> > > >>> as
> > > >>> > >> > WindowStore does:
> > > >>> > >> >
> > > >>> > >> > Key-based ranges: reverseRange(), reverseAll()
> > > >>> > >> > Time-based ranges: backwardFetch()
> > > >>> > >> >
> > > >>> > >> > Then, key-based changes apply to KeyValueStore, and time-based
> > > >>> changes
> > > >>> > >> to
> > > >>> > >> > Window and Session stores.
> > > >>> > >> >
> > > >>> > >> > Let me know if you have any questions.
> > > >>> > >> >
> > > >>> > >> > Thanks,
> > > >>> > >> > Jorge.
> > > >>> > >> >
> > > >>> > >> >
> > > >>> > >> > On Tue, Jun 16, 2020 at 12:47 AM Jorge Esteban Quilcate Otoya
> > <
> > > >>> > >> > quilcate.jo...@gmail.com> wrote:
> > > >>> > >> >
> > > >>> > >> > > 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
> > > >>> > >> the
> > > >>> > >> > > KIP's scope:
> > > >>> > >> > >
> > > >>> > >> > > 1. There are 2 ranges been exposed by different stores:
> > > >>> > >> > >
> > > >>> > >> > > a. Key Range
> > > >>> > >> > > b. Timestamp Range
> > > >>> > >> > >
> > > >>> > >> > > So far, we have discussed covering both.
> > > >>> > >> > >
> > > >>> > >> > > 2. Key Range functions do not provide ordering guarantees by
> > > >>> design:
> > > >>> > >> > >
> > > >>> > >> > > ```ReadOnlyKeyValueStore.java
> > > >>> > >> > > /**
> > > >>> > >> > > * Get an iterator over a given range of keys. This
> > > >>> iterator must
> > > >>> > >> be
> > > >>> > >> > > closed after use.
> > > >>> > >> > > * The returned iterator must be safe from {@link
> > > >>> > >> > > java.util.ConcurrentModificationException}s
> > > >>> > >> > > * and must not return null values. No ordering
> > guarantees
> > > >>> are
> > > >>> > >> > > provided.
> > > >>> > >> > > * ...
> > > >>> > >> > > */
> > > >>> > >> > > KeyValueIterator<K, V> range(K from, K to);
> > > >>> > >> > > ```
> > > >>> > >> > >
> > > >>> > >> > > Therefore, I'd propose removing Key range operations from
> > the
> > > >>> scope.
> > > >>> > >> > >
> > > >>> > >> > > 3. Timestamp Range operations happen at the SegmentsStore
> > level
> > > >>> > >> (internal)
> > > >>> > >> > > API
> > > >>> > >> > >
> > > >>> > >> > > AFAICT, Segments wrappers handle all Timestamp ranges
> > queries.
> > > >>> > >> > >
> > > >>> > >> > > I'd propose extending `Segments#segments(timeFrom, timeTo,
> > > >>> backwards)`
> > > >>> > >> > > with a flag for backwards operations.
> > > >>> > >> > >
> > > >>> > >> > > As segments returned will be processed backwards, I'm not
> > > >>> extending
> > > >>> > >> > > KeyValueStores to query each segment backwards as previous
> > > >>> point 2.
> > > >>> > >> > >
> > > >>> > >> > > 4. Extend WindowStores implementations with a new
> > > >>> > >> > > WindowBackwardStore/ReadOnlyBackwardStore:
> > > >>> > >> > >
> > > >>> > >> > > ```java
> > > >>> > >> > > public interface ReadOnlyWindowBackwardStore<K, V> {
> > > >>> > >> > > WindowStoreIterator<V> backwardFetch(K key, Instant
> > from,
> > > >>> Instant
> > > >>> > >> to)
> > > >>> > >> > > throws IllegalArgumentException;
> > > >>> > >> > >
> > > >>> > >> > > KeyValueIterator<Windowed<K>, V> backwardFetch(K from,
> > K to,
> > > >>> > >> Instant
> > > >>> > >> > > fromTime, Instant toTime)
> > > >>> > >> > > throws IllegalArgumentException;
> > > >>> > >> > >
> > > >>> > >> > > KeyValueIterator<Windowed<K>, V>
> > backwardFetchAll(Instant
> > > >>> from,
> > > >>> > >> > > Instant to) throws IllegalArgumentException;
> > > >>> > >> > > ```
> > > >>> > >> > >
> > > >>> > >> > > 5. SessionStore is a bit different as it has fetch/find
> > sessions
> > > >>> > >> spread
> > > >>> > >> > > between SessionStore and ReadOnlySessionStore.
> > > >>> > >> > >
> > > >>> > >> > > I'd propose a new interface `SessionBackwardStore` to expose
> > > >>> backward
> > > >>> > >> find
> > > >>> > >> > > operations:
> > > >>> > >> > >
> > > >>> > >> > > ```java
> > > >>> > >> > > public interface SessionBackwardStore<K, AGG> {
> > > >>> > >> > > KeyValueIterator<Windowed<K>, AGG>
> > > >>> backwardFindSessions(final K
> > > >>> > >> key,
> > > >>> > >> > > final long earliestSessionEndTime, final long
> > > >>> latestSessionStartTime);
> > > >>> > >> > >
> > > >>> > >> > > KeyValueIterator<Windowed<K>, AGG>
> > > >>> backwardFindSessions(final K
> > > >>> > >> > > keyFrom, final K keyTo, final long earliestSessionEndTime,
> > > >>> final long
> > > >>> > >> > > latestSessionStartTime);
> > > >>> > >> > > }
> > > >>> > >> > > ```
> > > >>> > >> > >
> > > >>> > >> > > If this understanding is correct I'll proceed to update the
> > KIP
> > > >>> based
> > > >>> > >> on
> > > >>> > >> > > this.
> > > >>> > >> > >
> > > >>> > >> > > Looking forward to your feedback.
> > > >>> > >> > >
> > > >>> > >> > > Thanks,
> > > >>> > >> > > Jorge.
> > > >>> > >> > >
> > > >>> > >> > > On Fri, May 29, 2020 at 3:32 AM Matthias J. Sax <
> > > >>> mj...@apache.org>
> > > >>> > >> wrote:
> > > >>> > >> > >
> > > >>> > >> > >> 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 is
> > > >>> > >> not
> > > >>> > >> > >> ideal either.
> > > >>> > >> > >>
> > > >>> > >> > >> Thus, I would like to put out a new idea: We could add a
> > new
> > > >>> > >> interface
> > > >>> > >> > >> that offers new methods that return revers iterators.
> > > >>> > >> > >>
> > > >>> > >> > >> The KIP already proposes to add `reverseAll()` and it seems
> > > >>> backward
> > > >>> > >> > >> incompatible to just add this method to
> > > >>> `ReadOnlyKeyValueStore` and
> > > >>> > >> > >> `ReadOnlyWindowStore`. I don't think we could provide a
> > useful
> > > >>> > >> default
> > > >>> > >> > >> implementation for custom stores and thus either break
> > > >>> compatibility
> > > >>> > >> or
> > > >>> > >> > >> need add a default that just throws an exception. Neither
> > > >>> seems to
> > > >>> > >> be a
> > > >>> > >> > >> good option.
> > > >>> > >> > >>
> > > >>> > >> > >> Using a new interface avoid this issue and allows users
> > > >>> implementing
> > > >>> > >> > >> custom stores to opt-in by adding the interface to their
> > > >>> stores.
> > > >>> > >> > >> Furthermore, we don't need any config. In the end, we
> > > >>> encapsulte the
> > > >>> > >> > >> change into the store, and our runtime is agnostic to it
> > (as it
> > > >>> > >> should
> > > >>> > >> > >> be).
> > > >>> > >> > >>
> > > >>> > >> > >> The hierarchy becomes a little complex (but uses would not
> > > >>> really see
> > > >>> > >> > >> the complexity):
> > > >>> > >> > >>
> > > >>> > >> > >> // exsiting
> > > >>> > >> > >> ReadOnlyKeyValueStore
> > > >>> > >> > >> KeyValueStore extend StateStore, ReadOnlyKeyValueStore
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> // helper interface; users don't care
> > > >>> > >> > >> // need similar ones for other stores
> > > >>> > >> > >> ReverseReadOnlyKeyValueStore {
> > > >>> > >> > >> KeyValueIterator<K, V> reverseRange(K from, K to);
> > > >>> > >> > >> KeyValueIterator<K, V> reverseAll();
> > > >>> > >> > >> }
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> // two new user facing interfaces for kv-store
> > > >>> > >> > >> // need similar ones for other stores
> > > >>> > >> > >> ReadOnlyKeyValueStoreWithReverseIterators extends
> > > >>> > >> ReadOnlyKeyValueStore,
> > > >>> > >> > >> ReverseReadOnlyKeyValueStore
> > > >>> > >> > >>
> > > >>> > >> > >> KeyValueStoreWithReverseIterators extends KeyValueStore,
> > > >>> > >> > >> ReverseReadOnlyKeyValueStore
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> // updated (also internal)
> > > >>> > >> > >> // also need to update other built-in stores
> > > >>> > >> > >> RocksDB implements KeyValueStoreWithReverseIterators,
> > > >>> > >> BulkLoadingStore
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> In the end, user would only care about the two (for
> > kv-case)
> > > >>> new
> > > >>> > >> > >> interface that offer revers iterator (read only and
> > regular)
> > > >>> and can
> > > >>> > >> > >> cast stores accordingly in their Processors/Transformers or
> > > >>> via IQ.
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> Btw: if we add revers iterator for KeyValue and Window
> > store,
> > > >>> should
> > > >>> > >> we
> > > >>> > >> > >> do the same for Session store?
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> This might be more code to write, but I believe it
> > provides the
> > > >>> > >> better
> > > >>> > >> > >> user experience. Thoughts?
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> -Matthias
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> > >> On 5/26/20 8:47 PM, John Roesler wrote:
> > > >>> > >> > >> > 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!
> > > >>> > >> > >> >>
> > > >>> > >> > >> >> 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.
> > > >>> > >> > >> >>>
> > > >>> > >> > >> >>>
> > > >>> > >> > >> >>>
> > > >>> > >> > >> >>> On Fri, May 22, 2020 at 4:22 AM Sophie Blee-Goldman <
> > > >>> > >> > >> sop...@confluent.io>
> > > >>> > >> > >> >>> 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 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
> > > >>> well-defined
> > > >>> > >> > >> ordering
> > > >>> > >> > >> >>>> such
> > > >>> > >> > >> >>>> that
> > > >>> > >> > >> >>>> you can't determine the MAX_KEY, then you probably
> > don't
> > > >>> care
> > > >>> > >> about
> > > >>> > >> > >> the
> > > >>> > >> > >> >>>> iterator order anyway.
> > > >>> > >> > >> >>>>
> > > >>> > >> > >> >>>> But with variable-length keys, there is no MAX_KEY.
> > If
> > > >>> all your
> > > >>> > >> > >> keys were
> > > >>> > >> > >> >>>> just
> > > >>> > >> > >> >>>> of the form 'a', 'aa', 'aaaaa', 'aaaaaaa' then in fact
> > > >>> the only
> > > >>> > >> way
> > > >>> > >> > >> to
> > > >>> > >> > >> >>>> figure out the
> > > >>> > >> > >> >>>> maximum key in the store is by using `all()` -- and
> > > >>> without a
> > > >>> > >> reverse
> > > >>> > >> > >> >>>> iterator, you're
> > > >>> > >> > >> >>>> doomed to iterate through every single key just to
> > answer
> > > >>> that
> > > >>> > >> simple
> > > >>> > >> > >> >>>> question.
> > > >>> > >> > >> >>>>
> > > >>> > >> > >> >>>> That said, I still think determining the iterator
> > order
> > > >>> based
> > > >>> > >> on the
> > > >>> > >> > >> >>>> to/from bytes
> > > >>> > >> > >> >>>> makes a lot of intuitive sense and gives the API a
> > nice
> > > >>> > >> symmetry.
> > > >>> > >> > >> What if
> > > >>> > >> > >> >>>> we
> > > >>> > >> > >> >>>> solved the `all()` problem by just giving `all()` a
> > > >>> reverse
> > > >>> > >> form to
> > > >>> > >> > >> >>>> complement it?
> > > >>> > >> > >> >>>> Ie we would have `all()` and `reverseAll()`, or
> > something
> > > >>> to
> > > >>> > >> that
> > > >>> > >> > >> effect.
> > > >>> > >> > >> >>>>
> > > >>> > >> > >> >>>> On Thu, May 21, 2020 at 3:41 PM Jorge Esteban Quilcate
> > > >>> Otoya <
> > > >>> > >> > >> >>>> quilcate.jo...@gmail.com> wrote:
> > > >>> > >> > >> >>>>
> > > >>> > >> > >> >>>>> 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` enum it apply to
> > both.
> > > >>> > >> > >> >>>>>
> > > >>> > >> > >> >>>>> The only issue I've found while reviewing the KIP is
> > that
> > > >>> > >> `all()`
> > > >>> > >> > >> won't
> > > >>> > >> > >> >>>> fit
> > > >>> > >> > >> >>>>> within this approach.
> > > >>> > >> > >> >>>>>
> > > >>> > >> > >> >>>>> We could remove it from the KIP and argue that for
> > > >>> WindowStore,
> > > >>> > >> > >> >>>>> `fetchAll(0, Long.MAX_VALUE)` can be used to get all
> > in
> > > >>> reverse
> > > >>> > >> > >> order,
> > > >>> > >> > >> >>>> and
> > > >>> > >> > >> >>>>> for KeyValueStore, no ordering guarantees are
> > provided.
> > > >>> > >> > >> >>>>>
> > > >>> > >> > >> >>>>> If there is consensus with this changes, I will go
> > and
> > > >>> update
> > > >>> > >> the
> > > >>> > >> > >> KIP.
> > > >>> > >> > >> >>>>>
> > > >>> > >> > >> >>>>> On Thu, May 21, 2020 at 3:33 PM John Roesler <
> > > >>> > >> vvcep...@apache.org>
> > > >>> > >> > >> >>>> wrote:
> > > >>> > >> > >> >>>>>
> > > >>> > >> > >> >>>>>> 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 seems like there are two options here, one is to
> > add
> > > >>> a new
> > > >>> > >> > >> config:
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>> StreamsConfig.ENABLE_BACKWARDS_ITERATION =
> > > >>> > >> > >> >>>>>> "enable.backwards.iteration
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>> Or we can add a feature flag in Materialized, like
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>> Materialized.enableBackwardsIteration()
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>> I think I'd personally lean toward the config, for
> > the
> > > >>> > >> following
> > > >>> > >> > >> >>>> reason.
> > > >>> > >> > >> >>>>>> The concern that Sophie raised is that someone's
> > > >>> program may
> > > >>> > >> depend
> > > >>> > >> > >> >>>>>> on the existing contract of getting an empty
> > iterator.
> > > >>> We
> > > >>> > >> don't
> > > >>> > >> > >> want
> > > >>> > >> > >> >>>> to
> > > >>> > >> > >> >>>>>> switch behavior when they aren't expecting it, so we
> > > >>> provide
> > > >>> > >> them a
> > > >>> > >> > >> >>>>>> config to assert that they _are_ expecting the new
> > > >>> behavior,
> > > >>> > >> which
> > > >>> > >> > >> >>>>>> means they take responsibility for updating their
> > code
> > > >>> to
> > > >>> > >> expect
> > > >>> > >> > >> the
> > > >>> > >> > >> >>>> new
> > > >>> > >> > >> >>>>>> behavior.
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>> There doesn't seem to be a reason to offer a choice
> > of
> > > >>> > >> behaviors
> > > >>> > >> > >> on a
> > > >>> > >> > >> >>>>>> per-query, or per-store basis. We just want people
> > to
> > > >>> be not
> > > >>> > >> > >> surprised
> > > >>> > >> > >> >>>>>> by this change in general.
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>> What do you think?
> > > >>> > >> > >> >>>>>> Thanks,
> > > >>> > >> > >> >>>>>> -John
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>> On Wed, May 20, 2020, at 17:37, Jorge Quilcate
> > wrote:
> > > >>> > >> > >> >>>>>>> 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 later operations become affected; which might
> > be
> > > >>> ok for
> > > >>> > >> most
> > > >>> > >> > >> >>>> (all?)
> > > >>> > >> > >> >>>>>>> cases. I can't think of an scenario where this
> > would
> > > >>> be an
> > > >>> > >> issue,
> > > >>> > >> > >> >>>> just
> > > >>> > >> > >> >>>>>>> want to point this out.
> > > >>> > >> > >> >>>>>>>
> > > >>> > >> > >> >>>>>>> If moving to this approach, I'd like to check if I
> > got
> > > >>> this
> > > >>> > >> right
> > > >>> > >> > >> >>>>> before
> > > >>> > >> > >> >>>>>>> updating the KIP:
> > > >>> > >> > >> >>>>>>>
> > > >>> > >> > >> >>>>>>> - only `StateStore` will change by having a new
> > method:
> > > >>> > >> > >> >>>>>>> `backwardIteration()`, `false` by default to keep
> > > >>> things
> > > >>> > >> > >> compatible.
> > > >>> > >> > >> >>>>>>> - then all `*Stores` will have to update their
> > > >>> implementation
> > > >>> > >> > >> based
> > > >>> > >> > >> >>>> on
> > > >>> > >> > >> >>>>>>> this flag.
> > > >>> > >> > >> >>>>>>>
> > > >>> > >> > >> >>>>>>>
> > > >>> > >> > >> >>>>>>> On 20/05/2020 21:02, Sophie Blee-Goldman wrote:
> > > >>> > >> > >> >>>>>>>>> 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 never return results with decreasing keys.
> > The
> > > >>> empty
> > > >>> > >> > >> iterator
> > > >>> > >> > >> >>>>>> wouldn't
> > > >>> > >> > >> >>>>>>>> break that contract, but of course a surprise
> > reverse
> > > >>> > >> iterator
> > > >>> > >> > >> >>>> would.
> > > >>> > >> > >> >>>>>>>>
> > > >>> > >> > >> >>>>>>>> FWIW I actually am in favor of automatically
> > > >>> converting to a
> > > >>> > >> > >> >>>> reverse
> > > >>> > >> > >> >>>>>>>> iterator,
> > > >>> > >> > >> >>>>>>>> I just thought we should consider whether this
> > should
> > > >>> be
> > > >>> > >> off by
> > > >>> > >> > >> >>>>>> default or
> > > >>> > >> > >> >>>>>>>> even possible to disable at all.
> > > >>> > >> > >> >>>>>>>>
> > > >>> > >> > >> >>>>>>>> On Tue, May 19, 2020 at 7:42 PM John Roesler <
> > > >>> > >> > >> vvcep...@apache.org
> > > >>> > >> > >> >>>>>
> > > >>> > >> > >> >>>>>> wrote:
> > > >>> > >> > >> >>>>>>>>
> > > >>> > >> > >> >>>>>>>>> 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 fancy".
> > > >>> > >> Sometimes, it's
> > > >>> > >> > >> >>>>>> better
> > > >>> > >> > >> >>>>>>>>> to be explicit instead of "elegant", if we think
> > more
> > > >>> > >> people
> > > >>> > >> > >> >>>> will be
> > > >>> > >> > >> >>>>>>>>> confused
> > > >>> > >> > >> >>>>>>>>> than not.
> > > >>> > >> > >> >>>>>>>>>
> > > >>> > >> > >> >>>>>>>>> I really don't think that there's any danger of
> > > >>> "relying
> > > >>> > >> on a
> > > >>> > >> > >> >>>> bug"
> > > >>> > >> > >> >>>>>> here,
> > > >>> > >> > >> >>>>>>>>> although
> > > >>> > >> > >> >>>>>>>>> people certainly could be relying on current
> > > >>> behavior. One
> > > >>> > >> thing
> > > >>> > >> > >> >>>> to
> > > >>> > >> > >> >>>>> be
> > > >>> > >> > >> >>>>>>>>> clear
> > > >>> > >> > >> >>>>>>>>> about (which I just left a more detailed comment
> > in
> > > >>> > >> KAFKA-8159
> > > >>> > >> > >> >>>>> about)
> > > >>> > >> > >> >>>>>> is
> > > >>> > >> > >> >>>>>>>>> that
> > > >>> > >> > >> >>>>>>>>> when we say something like key1 > key2, this
> > > >>> ordering is
> > > >>> > >> defined
> > > >>> > >> > >> >>>> by
> > > >>> > >> > >> >>>>>> the
> > > >>> > >> > >> >>>>>>>>> serde's output and nothing else.
> > > >>> > >> > >> >>>>>>>>>
> > > >>> > >> > >> >>>>>>>>> Currently, thanks to your fix in
> > > >>> > >> > >> >>>>>> https://github.com/apache/kafka/pull/6521
> > > >>> > >> > >> >>>>>>>>> ,
> > > >>> > >> > >> >>>>>>>>> the store contract is that for range scans, if
> > from
> > > >>> > to,
> > > >>> > >> then
> > > >>> > >> > >> >>>> the
> > > >>> > >> > >> >>>>>> store
> > > >>> > >> > >> >>>>>>>>> must
> > > >>> > >> > >> >>>>>>>>> return an empty iterator. 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.
> > > >>> > >> > >> >>>>>>>>>
> > > >>> > >> > >> >>>>>>>>> My counterproposal was to actually change this
> > > >>> contract to
> > > >>> > >> say
> > > >>> > >> > >> >>>> that
> > > >>> > >> > >> >>>>>> the
> > > >>> > >> > >> >>>>>>>>> store
> > > >>> > >> > >> >>>>>>>>> must return an iterator over the keys in that
> > range,
> > > >>> but
> > > >>> > >> in the
> > > >>> > >> > >> >>>>>> reverse
> > > >>> > >> > >> >>>>>>>>> order.
> > > >>> > >> > >> >>>>>>>>> So, in addition to considering whether this idea
> > is
> > > >>> "too
> > > >>> > >> fancy"
> > > >>> > >> > >> >>>> (aka
> > > >>> > >> > >> >>>>>>>>> confusing),
> > > >>> > >> > >> >>>>>>>>> we should also consider the likelihood of
> > breaking an
> > > >>> > >> existing
> > > >>> > >> > >> >>>>>> program with
> > > >>> > >> > >> >>>>>>>>> this behavior/contract change.
> > > >>> > >> > >> >>>>>>>>>
> > > >>> > >> > >> >>>>>>>>> To echo your clarification, I'm also not
> > advocating
> > > >>> > >> strongly in
> > > >>> > >> > >> >>>>> favor
> > > >>> > >> > >> >>>>>> of my
> > > >>> > >> > >> >>>>>>>>> proposal. I just wanted to present it for
> > > >>> consideration
> > > >>> > >> > >> alongside
> > > >>> > >> > >> >>>>>> Jorge's
> > > >>> > >> > >> >>>>>>>>> original one.
> > > >>> > >> > >> >>>>>>>>>
> > > >>> > >> > >> >>>>>>>>> Thanks for raising these very good points,
> > > >>> > >> > >> >>>>>>>>> -John
> > > >>> > >> > >> >>>>>>>>>
> > > >>> > >> > >> >>>>>>>>> On Tue, May 19, 2020, at 20:49, Sophie
> > Blee-Goldman
> > > >>> wrote:
> > > >>> > >> > >> >>>>>>>>>>> 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
> > > >>> > >> as the
> > > >>> > >> > >> >>>>> above,
> > > >>> > >> > >> >>>>>>>>>> and/or
> > > >>> > >> > >> >>>>>>>>>> they rely on getting results in increasing order
> > > >>> (of the
> > > >>> > >> keys)
> > > >>> > >> > >> >>>> even
> > > >>> > >> > >> >>>>>> when
> > > >>> > >> > >> >>>>>>>>>> to < from. Maybe they're relying on the fact
> > that
> > > >>> the
> > > >>> > >> range
> > > >>> > >> > >> >>>> query
> > > >>> > >> > >> >>>>>> returns
> > > >>> > >> > >> >>>>>>>>>> nothing in that case.
> > > >>> > >> > >> >>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>> Not sure if that qualifies as relying on a bug
> > or
> > > >>> not,
> > > >>> > >> but in
> > > >>> > >> > >> >>>> that
> > > >>> > >> > >> >>>>>> past
> > > >>> > >> > >> >>>>>>>>>> we've
> > > >>> > >> > >> >>>>>>>>>> taken the stance that we should not break
> > > >>> compatibility
> > > >>> > >> even if
> > > >>> > >> > >> >>>> the
> > > >>> > >> > >> >>>>>> user
> > > >>> > >> > >> >>>>>>>>>> was relying on bugs or unintentional behavior.
> > > >>> > >> > >> >>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>> Just to clarify I'm not advocating strongly
> > against
> > > >>> this
> > > >>> > >> > >> >>>> proposal,
> > > >>> > >> > >> >>>>>> just
> > > >>> > >> > >> >>>>>>>>>> laying
> > > >>> > >> > >> >>>>>>>>>> out some considerations we should take into
> > > >>> account. At
> > > >>> > >> the end
> > > >>> > >> > >> >>>> of
> > > >>> > >> > >> >>>>>> the
> > > >>> > >> > >> >>>>>>>>> day
> > > >>> > >> > >> >>>>>>>>>> we should do what's right rather than maintain
> > > >>> > >> compatibility
> > > >>> > >> > >> >>>> with
> > > >>> > >> > >> >>>>>>>>> existing
> > > >>> > >> > >> >>>>>>>>>> bugs, but sometimes there's a reasonable middle
> > > >>> ground.
> > > >>> > >> > >> >>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>> On Tue, May 19, 2020 at 6:15 PM John Roesler <
> > > >>> > >> > >> >>>> vvcep...@apache.org>
> > > >>> > >> > >> >>>>>>>>> wrote:
> > > >>> > >> > >> >>>>>>>>>>> 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 mind?
> > > >>> > >> > >> >>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>> Thanks,
> > > >>> > >> > >> >>>>>>>>>>> John
> > > >>> > >> > >> >>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>> On Tue, May 19, 2020, at 19:10, Sophie
> > Blee-Goldman
> > > >>> > >> wrote:
> > > >>> > >> > >> >>>>>>>>>>>> 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
> > > >>> > >> > >> >>>>>>>>>>>> <
> > https://issues.apache.org/jira/browse/KAFKA-8159
> > > >>> >
> > > >>> > >> with the
> > > >>> > >> > >> >>>>>> built-in
> > > >>> > >> > >> >>>>>>>>>>> signed
> > > >>> > >> > >> >>>>>>>>>>>> number serdes.
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> This trick would actually fix the problem for
> > > >>> > >> > >> >>>> negative-negative
> > > >>> > >> > >> >>>>>>>>> queries
> > > >>> > >> > >> >>>>>>>>>>>> (ie where to & from are negative) but would
> > cause
> > > >>> > >> > >> undetectable
> > > >>> > >> > >> >>>>>>>>>>>> incorrect results for negative-positive
> > queries.
> > > >>> For
> > > >>> > >> example,
> > > >>> > >> > >> >>>> say
> > > >>> > >> > >> >>>>>> you
> > > >>> > >> > >> >>>>>>>>>>>> call #range with from = -1 and to = 1, using
> > the
> > > >>> Short
> > > >>> > >> > >> serdes.
> > > >>> > >> > >> >>>>> The
> > > >>> > >> > >> >>>>>>>>>>>> serialized bytes for that are
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> from = 1111111111111111
> > > >>> > >> > >> >>>>>>>>>>>> to = 0000000000000001
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> so we would end up flipping those and
> > iterating
> > > >>> over
> > > >>> > >> all keys
> > > >>> > >> > >> >>>>> from
> > > >>> > >> > >> >>>>>>>>>>>> 0000000000000001 to 1111111111111111.
> > Iterating in
> > > >>> > >> > >> >>>>> lexicographical
> > > >>> > >> > >> >>>>>>>>>>>> order means we would iterate over every key in
> > > >>> the space
> > > >>> > >> > >> >>>> *except*
> > > >>> > >> > >> >>>>>> for
> > > >>> > >> > >> >>>>>>>>>>>> 0, but 0 is actually the *only* other key we
> > > >>> meant to be
> > > >>> > >> > >> >>>> included
> > > >>> > >> > >> >>>>>> in
> > > >>> > >> > >> >>>>>>>>> the
> > > >>> > >> > >> >>>>>>>>>>>> range query.
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> Currently we just log a warning and return an
> > > >>> empty
> > > >>> > >> iterator
> > > >>> > >> > >> >>>> when
> > > >>> > >> > >> >>>>>>>>>>>> to < from, which is obviously also incorrect
> > but
> > > >>> feels
> > > >>> > >> > >> >>>> slightly
> > > >>> > >> > >> >>>>>> more
> > > >>> > >> > >> >>>>>>>>>>>> palatable. If we start automatically
> > converting to
> > > >>> > >> reverse
> > > >>> > >> > >> >>>>> queries
> > > >>> > >> > >> >>>>>> we
> > > >>> > >> > >> >>>>>>>>>>>> can't even log a warning in this case unless
> > we
> > > >>> wanted
> > > >>> > >> to log
> > > >>> > >> > >> >>>> a
> > > >>> > >> > >> >>>>>>>>> warning
> > > >>> > >> > >> >>>>>>>>>>>> every time, which would be weird to do for a
> > valid
> > > >>> > >> usage of a
> > > >>> > >> > >> >>>> new
> > > >>> > >> > >> >>>>>>>>>>>> feature.
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> All that said, I still like the idea overall.
> > Off
> > > >>> the
> > > >>> > >> top of
> > > >>> > >> > >> >>>> my
> > > >>> > >> > >> >>>>>> head
> > > >>> > >> > >> >>>>>>>>> I
> > > >>> > >> > >> >>>>>>>>>>> guess
> > > >>> > >> > >> >>>>>>>>>>>> we could add a store config to enable/disable
> > > >>> automatic
> > > >>> > >> > >> >>>> reverse
> > > >>> > >> > >> >>>>>>>>>>> iteration,
> > > >>> > >> > >> >>>>>>>>>>>> which is off by default?
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> Thanks for the KIP! This will be a nice
> > addition
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> Sophie
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>> On Tue, May 19, 2020 at 3:21 PM John Roesler <
> > > >>> > >> > >> >>>>> vvcep...@apache.org>
> > > >>> > >> > >> >>>>>>>>>>> wrote:
> > > >>> > >> > >> >>>>>>>>>>>>> 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 from "Z" to "A" would
> > be a
> > > >>> > >> backward
> > > >>> > >> > >> >>>> one?
> > > >>> > >> > >> >>>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>>> Thanks,
> > > >>> > >> > >> >>>>>>>>>>>>> -John
> > > >>> > >> > >> >>>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>>> On Tue, May 19, 2020, at 16:20, Jorge
> > Quilcate
> > > >>> wrote:
> > > >>> > >> > >> >>>>>>>>>>>>>> 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.
> > > >>> > >> > >> >>>>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>>>>
> > > >>> > >> > >> >>>>>>>>>>>>>> Attachments:
> > > >>> > >> > >> >>>>>>>>>>>>>> * 0x5F2C6E22064982DF.asc
> > > >>> > >> > >> >>>>>>>
> > > >>> > >> > >> >>>>>>>
> > > >>> > >> > >> >>>>>>> Attachments:
> > > >>> > >> > >> >>>>>>> * 0x5F2C6E22064982DF.asc
> > > >>> > >> > >> >>>>>>
> > > >>> > >> > >> >>>>>
> > > >>> > >> > >> >>>>
> > > >>> > >> > >> >>>
> > > >>> > >> > >> >>
> > > >>> > >> > >>
> > > >>> > >> > >>
> > > >>> > >> >
> > > >>> > >>
> > > >>> > >
> > > >>> >
> > > >>>
> > > >>
> > >
> >
>
