KAFKA-4064 Add support for infinite endpoints for range queries in Kafka Streams KV stores

[theduderog]

@guozhangwang

I had to fix the bug with in-memory ranges being exclusive and RocksDB being inclusive to get meaningful tests to work.

[dguy]

@theduderog you have a checkstyle failure

[dguy]

After seeing the API changes, i'm wondering if it would be better if we have a single function range(Range<K> range) - and have a class sth like:

class Range<K>
   K from;
   K to;
   ...
   static <K> from(K key) ...
   static <K> until(K key) ..
  // etc

It at least means we can support everything with a single method on the KeyValuStore interface.
Interested in hearing what @guozhangwang @miguno @enothereska @mjsax think

[miguno]

I don't have a strong opinion.

I see the benefits of a single range(Range<K> range) method. Its drawback is that users have to construct a new instance of Range<K> every time, which is more cumbersome in terms of using the API.

So:

store.rangeFrom(3)

versus

store.range(Range.from(3))

Perhaps a question to ask is: Would we need a separate Range class because we expect to add more functions than what rangeFrom(K) and rangeUntil(K) would cover?

[theduderog]

cc @ijuma as well

[ijuma]

I prefer the method taking a Range personally (it is why I suggested that offline when the discussion was taking place). It is easy to add convenience methods later, if necessary, but hard to remove methods from the interface.

[guozhangwang]

There are some jenkins failures that seems related to the changes.

[theduderog]

@guozhangwang I'll fix those. What you think about the API?

[dguy]

@theduderog - sorry we haven't progressed this at all. Besides the conflicts. I'm in favor of the API as I mentioned above (Obviously!) @enothereska @mjsax @guozhangwang - thoughts?

[enothereska]

Yeah good idea. Let's push to get this in asap. @theduderog could you resolve the conflict and then we can take it from there. Sorry for the delay.

[mjsax]

ping @theduderog

[theduderog]

retest this please

[theduderog]

Is range() supposed to be inclusive or exclusive of the endpoint? So for, this PR makes them all inclusive.

The docs say inclusive, the RocksDB implementation is inclusive, but the InMemory implementation is exclusive.

Not to mentioned that Java SortedSet.subSet defaults to exclusive and seems like the more natural choice so it's likely what users expect.

[guozhangwang]

Re: range() I think we can make it consistent as inclusive everywhere, the InMemoryStore's range function seems to be a bug itself. Could you fix it along with this PR?

Re: API, I think I also agree with @dguy and @ijuma on the new Range class that

class Range<K>
   private K from;
   private K to;
   ...
   static <K> from(K key) ...
   static <K> until(K key) ..
   static <K> range(K keyFrom, K keyUtil) ..

which could be generalized later. At the same time we can keep the current function and reimplemented as Range.range(..).

[guozhangwang]

Made a pass over the implementation, LGTM overall.

[theduderog]

retest this please

[guozhangwang]

@theduderog retest this does not work for apache repos just FYI :P

[theduderog]

thanks, @guozhangwang. it may have just been a coincidence but it seemed to kick off right after i added that comment

[guozhangwang]

@theduderog Could you file a KIP for this API change as well?

[dguy]

I thought we were going to have a single range method that takes a Range class?

[theduderog]

@dguy I'll switch to that for the KIP

[mjsax]

Closing this PR due to inactivity.

[vvcephei]

FYI, this was implemented in #11120