Thanks for all the feedback folk! Responses inline. > Basically, I'm suggesting two things: first, call out in some way (perhaps the StoreTypeSpec javadocs) that each StoreTypeSpec is considered a public contract in itself and should outline any semantic guarantees it does, or does not, make. Second, we should add a note on ordering guarantees in the two OOTB specs: for RocksDB we assert that range queries will honor serialized byte ordering, whereas the InMemory flavor gives no ordering guarantee whatsoever at this time.
That makes sense to me Sophie! I'll make the changes to the KIP. And @Colt, yes I believe that would be the new javadoc for the generic ReadOnlyKeyValueStore. > However, I am wondering if we should close others gaps first? @Matthias, thanks for the review and thoughts! I think we should separate closing other gaps in the product from providing this as useful functionality to avoid feature creep so long as the API proposed here will be suitable for when we want to close those implementation gaps! My general proposal is that for things that are not customizable today by default.dsl.store they remain not customizable after this KIP. The good news is, however, that there's no reason why this cannot be extended to cover those in the future if we want to - see specifics below. Comments on the specifics below > In particular, this holds for the new versioned-store ... Should versioned stores also be covered by the KIP Is there a reason why we can't introduce a VersionedRocksDBStoreTypeSpec and if we ever support an in-memory an equivalent VersionedInMemoryRocksDBStoreTypeSpec? If so, then there would not need to be any additional changes to the API proposed in this KIP. > For `suppress()` it's actually other way around we only have an in-memory implementation. Do you aim to allow custom stores for `suppress()`, too? We have three options here: 1) we can decide to maintain existing behavior and use the in-memory implementation for all stores (not even going through the API at all) 2a) we can introduce a new parameter to the KeyValueParams class (boolean isTimeOrderedBuffer or something like that) and return an in-memory store in the implementation of RocksDBStoreTypeSpec (this maintains the existing behavior, and would allow us in the future to make the change to return a RocksDB store if we ever provide one) 2b) same as 2a but we throw an exception if the requested store type does not support that (this is backwards incompatible, and since ROCKS_DB is the default we probably shouldn't do this) My proposal for now is 1) because as of KIP-825 EmitStrategy#ON_WINDOW_CLOSE is the preferred way of suppressing and that is accounted for in this API already. > Last, I am not sure if the new parameter replacing the existing one is the best way to go? I'm happy either way, just let me know which you prefer - the discussion around CUSTOM is in the rejected alternatives but I'm happy to differ to whatever the project conventions are :) > If it's matches existing `ROCKS_DB` or `IN_MEMORY` we just process it as we do know, and if know we assume it's a fully qualified class name and try to instantiate it? Note that there is no functionality for this kind of thing in AbstractConfig (it's either a String validated enum or a class) so this would be a departure from convention. Again, I'm happy to implement that if it's preferred. > Also wondering how it would related to the existing `Stores` factory? StoreTypeSpec will depend on Stores factory - they're one layer removed. You can imagine that StoreTypeSpec is just a grouping of methods from the Stores factory into a convenient package for default configuration purposes. Thanks again for all the detailed thoughts Matthias! On Fri, Jul 21, 2023 at 11:50 AM Matthias J. Sax <mj...@apache.org> wrote: > Thanks for the KIP. Overall I like the idea to close this gap. > > However, I am wondering if we should close others gaps first? In > particular, IIRC, we have a few cases for which we only have a RocksDB > implementation for a store, and thus, adding an in-memory version for > these stores first, to make the current `IN_MEMORY` parameter work, > might be the first step? > > In particular, this holds for the new versioned-store (but I actually > believe the is some other internal store with no in-memory > implementation). -- For `suppress()` it's actually other way around we > we only have an in-memory implementation. Do you aim to allow custom > stores for `suppress()`, too? > > Btw: Should versioned stores also be covered by the KIP (ie, > `StoreTypeSpec`)? We did consider to add a new option `VERSIONED` to the > existing `default.dsl.store` config, but opted out for various reasons. > > Last, I am not sure if the new parameter replacing the existing one is > the best way to go? Did you put the idea to add `CUSTOM` to the existing > config into rejected alternative. Personally, I would prefer to add > `CUSTOM` as I would like to optimize to easy of use for the majority of > users (which don't implement a custom store), but only switch to > in-memory sometimes. -- As an alternative, you would also just extend > `dsl.default.store` (it's just a String) and allow to pass in anything. > If it's matches existing `ROCKS_DB` or `IN_MEMORY` we just process it as > we do know, and if know we assume it's a fully qualified class name and > try to instantiate it? -- Given that we plan to keep the store-enum, is > seems cleaner to keep the existing config and keep both the config and > enum aligned to each other? > > > It's just preliminary thought. I will need to go back an take a more > detailed look into the code to grok how the propose `StoreTypeSpec` > falls into place. Also wondering how it would related to the existing > `Stores` factory? > > -Matthias > > > On 7/21/23 6:45 AM, Colt McNealy wrote: > > Sophie— > > > > Thanks for chiming in here. +1 to the idea of specifying the ordering > > guarantees that we make in the StorageTypeSpec javadocs. > > > > Quick question then. Is the javadoc that says: > > > >> Order is not guaranteed as bytes lexicographical ordering might not > > represent key order. > > > > no longer correct, and should say: > > > >> Order guarantees depend on the underlying implementation of the > > ReadOnlyKeyValueStore. For more information, please consult the > > [StorageTypeSpec javadocs](....) > > > > Thanks, > > Colt McNealy > > > > *Founder, LittleHorse.dev* > > > > > > On Thu, Jul 20, 2023 at 9:28 PM Sophie Blee-Goldman < > ableegold...@gmail.com> > > wrote: > > > >> Hey Almog, first off, thanks for the KIP! I (and others) raised concerns > >> over how restrictive the default.dsl.store config would be if not > >> extendable to custom store types, especially given that this seems to be > >> the primary userbase of such a feature. At the time we didn't really > have > >> any better ideas for a clean way to achieve that, but what you proposed > >> makes a lot of sense to me. Happy to see a good solution to this, and > >> hopefully others will share my satisfaction :P > >> > >> I did have one quick piece of feedback which arose from an unrelated > >> question posed to the dev mailing list w/ subject line > >> "ReadOnlyKeyValueStore#range() > >> Semantics" > >> <https://lists.apache.org/thread/jbckmth8d3mcgg0rd670cpvsgwzqlwrm>. I > >> recommend checking out the full thread for context, but it made me think > >> about how we can leverage the new StoreTypeSpec concept as an answer to > the > >> long-standing question in Streams: where can we put guarantees of the > >> public contract for RocksDB (or other store implementations) when all > the > >> RocksDB stuff is technically internal. > >> > >> Basically, I'm suggesting two things: first, call out in some way > (perhaps > >> the StoreTypeSpec javadocs) that each StoreTypeSpec is considered a > public > >> contract in itself and should outline any semantic guarantees it does, > or > >> does not, make. Second, we should add a note on ordering guarantees in > the > >> two OOTB specs: for RocksDB we assert that range queries will honor > >> serialized byte ordering, whereas the InMemory flavor gives no ordering > >> guarantee whatsoever at this time. > >> > >> Thoughts? > >> > >> -Sophie > >> > >> On Thu, Jul 20, 2023 at 4:28 PM Almog Gavra <almog.ga...@gmail.com> > wrote: > >> > >>> Hi All, > >>> > >>> I would like to propose a KIP to expand support for default store types > >>> (KIP-591) to encompass custom store implementations: > >>> > >>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-954%3A+expand+default+DSL+store+configuration+to+custom+types > >>> > >>> Looking forward to your feedback! > >>> > >>> Cheers, > >>> Almog > >>> > >> > > >
