Thanks Almog, I read https://github.com/apache/kafka/pull/15061/files and I think the additional API is the right way of fixing it.
Guozhang On Fri, Dec 22, 2023 at 9:14 AM Almog Gavra <almog.ga...@gmail.com> wrote: > > Hello Everyone! I updated the KIP once more as a result of a bug > investigation - I added DslWindowParams#isTimestamped to the public API as > a result of https://issues.apache.org/jira/browse/KAFKA-16046. Please let > me know if there's any concerns with this addition. > > On Thu, Dec 14, 2023 at 5:40 PM Almog Gavra <almog.ga...@gmail.com> wrote: > > > Sorry for the late response to the late reply, hah! I didn't give any > > thought about how we would want to integrate this custom store supplier > > with querying of the stores. My initial intuition suggests that we'd > > probably want a separate API for that, or just recommend people to query > > their external stores outside of the context of Kafka Streams (with the > > understanding that there are fewer semantic guarantees). > > > > On Sat, Dec 2, 2023 at 9:38 AM Guozhang Wang <guozhang.wang...@gmail.com> > > wrote: > > > >> Hey Almog, > >> > >> Sorry for the late reply. > >> > >> Re: 2) above, maybe I'm just overthinking it. What I had in mind is > >> that when we have, say, a remote store impl customized by the users. > >> Besides being used inside the KS app itself, the user may try to > >> access the store instance outside the KS app as well? If that's the > >> case, maybe it's still worth having an interface from KS to expose the > >> store instance directly. > >> > >> > >> Guozhang > >> > >> > >> On Sun, Nov 19, 2023 at 5:26 PM Almog Gavra <almog.ga...@gmail.com> > >> wrote: > >> > > >> > Hello Guozhang, > >> > > >> > Thanks for the feedback! For 1 there are tests verifying this and I did > >> so > >> > manually as well, it does not reveal anything about the store types -- > >> just > >> > the names, so I think we're good there. I've put an example at the > >> bottom > >> > of this reply for people following the conversation. > >> > > >> > I'm not sure I understand your question about 2. What's the integration > >> > point with the actual store for this external component? What does that > >> > have to do with this PR/how does it differ from what's available today > >> > (with the default.dsl.store configuration)? In either scenario, getting > >> the > >> > actual instantiated store supplier must be done only after the topology > >> is > >> > built and rewritten (it can be passed in either via > >> > Materialized/StreamJoined in the DSL code, via TopologyConfig overrides > >> or > >> > in the global StreamsConfig passed in to KafkaStreams). Today, AFAIK, > >> this > >> > isn't possible (you can't get from the built topology the instantiated > >> > store supplier). > >> > > >> > Thanks, > >> > Almog > >> > > >> > ------------ > >> > > >> > Topologies: > >> > Sub-topology: 0 > >> > Source: KSTREAM-SOURCE-0000000000 (topics: [test_topic]) > >> > --> KSTREAM-TRANSFORMVALUES-0000000001 > >> > Processor: KSTREAM-TRANSFORMVALUES-0000000001 (stores: []) > >> > --> Aggregate-Prepare > >> > <-- KSTREAM-SOURCE-0000000000 > >> > Processor: Aggregate-Prepare (stores: []) > >> > --> KSTREAM-AGGREGATE-0000000003 > >> > <-- KSTREAM-TRANSFORMVALUES-0000000001 > >> > Processor: KSTREAM-AGGREGATE-0000000003 (stores: > >> > [Aggregate-Aggregate-Materialize]) > >> > --> Aggregate-Aggregate-ToOutputSchema > >> > <-- Aggregate-Prepare > >> > Processor: Aggregate-Aggregate-ToOutputSchema (stores: []) > >> > --> Aggregate-Project > >> > <-- KSTREAM-AGGREGATE-0000000003 > >> > Processor: Aggregate-Project (stores: []) > >> > --> KTABLE-TOSTREAM-0000000006 > >> > <-- Aggregate-Aggregate-ToOutputSchema > >> > Processor: KTABLE-TOSTREAM-0000000006 (stores: []) > >> > --> KSTREAM-SINK-0000000007 > >> > <-- Aggregate-Project > >> > Sink: KSTREAM-SINK-0000000007 (topic: S2) > >> > <-- KTABLE-TOSTREAM-0000000006 > >> > > >> > On Sat, Nov 18, 2023 at 6:05 PM Guozhang Wang < > >> guozhang.wang...@gmail.com> > >> > wrote: > >> > > >> > > Hello Almog, > >> > > > >> > > I left a comment in the PR before I got to read the newest updates > >> > > from this thread. My 2c: > >> > > > >> > > 1. I liked the idea of delaying the instantiation of StoreBuiler from > >> > > suppliers after the Topology is created. It has been a bit annoying > >> > > for many other features we were trying back then. The only thing is, > >> > > we need to check when we call Topology.describe() which gets a > >> > > TopologyDescription, does that reveal anything about the source of > >> > > truth store impl types already; if it does not, then we are good to > >> > > go. > >> > > > >> > > 2. I originally thought (and commented in the PR) that maybe we can > >> > > just add this new func "resolveDslStoreSuppliers" into StreamsConfig > >> > > directly and mark it as EVOLVING, because I was not clear that we are > >> > > trying to do 1) above. Now I'm leaning more towards what you proposed. > >> > > But I still have a question in mind: even after we've done > >> > > https://github.com/apache/kafka/pull/14548 later, don't we still need > >> > > some interface that user's can call to get the actual instantiated > >> > > store supplier for cases where some external custom logic, like an > >> > > external controller / scheduler which is developed by a different > >> > > group of people rather than the Streams app developers themselves, > >> > > that can only turn on certain features after learning the actual store > >> > > impl suppliers used? > >> > > > >> > > Guozhang > >> > > > >> > > On Sat, Nov 18, 2023 at 2:46 PM Almog Gavra <almog.ga...@gmail.com> > >> wrote: > >> > > > > >> > > > Hello Everyone - one more minor change to the KIP that came up > >> during > >> > > > implementation (reflected in the KIP itself). I will be adding the > >> method > >> > > > below to TopologyConfig. This allows us to determine whether or not > >> the > >> > > > DslStoreSuppliers was explicitly passed in via either > >> > > > DSL_STORE_SUPPLIERS_CLASS_CONFIG or DEFAULT_DSL_STORE_CONFIG (if it > >> was > >> > > not > >> > > > explicitly passed in, we will use the one that is configured in > >> > > > StreamsConfig, or the default value of RocksDBDslStoreSuppliers). > >> > > > > >> > > > See the discussion on the PR ( > >> > > > https://github.com/apache/kafka/pull/14648#discussion_r1394939779) > >> for > >> > > more > >> > > > context. Ideally this would be an internal utility method but > >> there's no > >> > > > clean way to get that done now, so the goal was to minimize the > >> surface > >> > > > area of what's being exposed (as opposed to exposing a generic > >> method > >> > > like > >> > > > isOverridden(String config). > >> > > > > >> > > > /** > >> > > > * @return the DslStoreSuppliers if the value was explicitly > >> configured > >> > > > (either by > >> > > > * {@link StreamsConfig#DEFAULT_DSL_STORE} or {@link > >> > > > StreamsConfig#DSL_STORE_SUPPLIERS_CLASS_CONFIG}) > >> > > > */ > >> > > > public Optional<DslStoreSuppliers> resolveDslStoreSuppliers(); > >> > > > > >> > > > On Fri, Nov 3, 2023 at 10:48 AM Matthias J. Sax <mj...@apache.org> > >> > > wrote: > >> > > > > >> > > > > Thanks. Will take a look into the PR. > >> > > > > > >> > > > > I don't have any objection to the goal; contrary! It's very > >> annoying > >> > > > > what we have right now, and if we can improve it, I am totally in > >> favor > >> > > > > of it. > >> > > > > > >> > > > > > >> > > > > -Matthias > >> > > > > > >> > > > > On 11/3/23 8:47 AM, Almog Gavra wrote: > >> > > > > > Good question :) I have a PR for it already here: > >> > > > > > https://github.com/apache/kafka/pull/14659. The concept is to > >> wrap > >> > > the > >> > > > > > suppliers with an interface that allows for delayed creation of > >> the > >> > > > > > StoreBuilder instead of creating the StoreBuilder from the > >> suppliers > >> > > > > right > >> > > > > > away. Happy to get on a quick call to outline the implementation > >> > > strategy > >> > > > > > if you'd like, but hopefully you have no objections to the goal! > >> > > > > > > >> > > > > > On Thu, Nov 2, 2023 at 8:44 PM Matthias J. Sax < > >> mj...@apache.org> > >> > > wrote: > >> > > > > > > >> > > > > >> Almog, > >> > > > > >> > >> > > > > >> can you explain how you intent to implement this change? It's > >> not > >> > > clear > >> > > > > >> to me, how we could do this? > >> > > > > >> > >> > > > > >> When we call `StreasmBuilder.build()` it will give us a already > >> > > fully > >> > > > > >> wired `Topology`, including all store suppliers needed. I > >> don't see > >> > > a > >> > > > > >> clean way how we could change the store supplier after the > >> fact? > >> > > > > >> > >> > > > > >> > >> > > > > >> -Matthias > >> > > > > >> > >> > > > > >> On 11/2/23 5:11 PM, Almog Gavra wrote: > >> > > > > >>> Hello everyone - I updated the KIP to also include the > >> following > >> > > > > >>> modification: > >> > > > > >>> > >> > > > > >>> Both the new dsl.store.suppliers.class and the old > >> > > default.dsl.store > >> > > > > >> will > >> > > > > >>> now respect the configurations when passed in via > >> > > > > >> KafkaStreams#new(Topology, > >> > > > > >>> StreamsConfig) (and other related constructors) instead of > >> only > >> > > being > >> > > > > >>> respected when passed in to the initial > >> > > > > StoreBuilder#new(TopologyConfig) > >> > > > > >>> (though it will be respected if passed in via the original > >> path as > >> > > > > well). > >> > > > > >>> > >> > > > > >>> I was honestly a bit shocked this wasn't the case with the > >> > > original KIP > >> > > > > >>> that introduced default.dsl.store! > >> > > > > >>> > >> > > > > >>> On Fri, Jul 28, 2023 at 4:55 PM Almog Gavra < > >> almog.ga...@gmail.com > >> > > > > >> > > > > >> wrote: > >> > > > > >>> > >> > > > > >>>> OK! I think I got everything, but I'll give the KIP another > >> read > >> > > with > >> > > > > >>>> fresh eyes later. Just a reminder that the voting is open, > >> so go > >> > > out > >> > > > > and > >> > > > > >>>> exercise your civic duty! ;) > >> > > > > >>>> > >> > > > > >>>> - Almog > >> > > > > >>>> > >> > > > > >>>> On Fri, Jul 28, 2023 at 10:38 AM Almog Gavra < > >> > > almog.ga...@gmail.com> > >> > > > > >>>> wrote: > >> > > > > >>>> > >> > > > > >>>>> Thanks Guozhang & Sophie: > >> > > > > >>>>> > >> > > > > >>>>> A2. Will clarify in the KIP > >> > > > > >>>>> A3. Will change back to the deprecated version! > >> > > > > >>>>> A5. Seems like I'm outnumbered... DslStoreSuppliers it is. > >> > > > > >>>>> > >> > > > > >>>>> Will update the KIP today. > >> > > > > >>>>> > >> > > > > >>>>> - Almog > >> > > > > >>>>> > >> > > > > >>>>> On Thu, Jul 27, 2023 at 12:42 PM Guozhang Wang < > >> > > > > >>>>> guozhang.wang...@gmail.com> wrote: > >> > > > > >>>>> > >> > > > > >>>>>> Yes, that sounds right to me. Thanks Sophie. > >> > > > > >>>>>> > >> > > > > >>>>>> On Thu, Jul 27, 2023 at 12:35 PM Sophie Blee-Goldman > >> > > > > >>>>>> <ableegold...@gmail.com> wrote: > >> > > > > >>>>>>> > >> > > > > >>>>>>> A2: Guozhang, just to close the book on the ListValue > >> store > >> > > thing, > >> > > > > I > >> > > > > >>>>>> fully > >> > > > > >>>>>>> agree it seems like overreach > >> > > > > >>>>>>> to expose/force this on users, especially if it's fully > >> > > internal > >> > > > > >>>>>> today. But > >> > > > > >>>>>>> just to make sure we're on the same > >> > > > > >>>>>>> page here, you're still ok with this KIP fixing the API > >> gap > >> > > that > >> > > > > >> exists > >> > > > > >>>>>>> today, in which these stores cannot be > >> > > > > >>>>>>> customized by the user at all? > >> > > > > >>>>>>> > >> > > > > >>>>>>> In other words, after this KIP, the new behavior for the > >> > > ListValue > >> > > > > >>>>>> store in > >> > > > > >>>>>>> a stream join will be: > >> > > > > >>>>>>> > >> > > > > >>>>>>> S1: First, check if the user passed in a > >> `DSLStoreSuppliers` > >> > > (or > >> > > > > >>>>>> whatever > >> > > > > >>>>>>> the name will be) to the > >> > > > > >>>>>>> StreamJoined config object, and use that to > >> obtain the > >> > > > > >>>>>>> KVStoreSupplier for this ListValue store > >> > > > > >>>>>>> > >> > > > > >>>>>>> S2: If none was provided, check if the user has set a > >> default > >> > > > > >>>>>>> DSLStoreSuppliers via the new config, > >> > > > > >>>>>>> and use that to get the KVStoreSupplier if so > >> > > > > >>>>>>> > >> > > > > >>>>>>> S3: If neither is set, fall back to the original logic as > >> it is > >> > > > > >> today, > >> > > > > >>>>>>> which is to pass in a KVStoreSupplier > >> > > > > >>>>>>> that is hard-coded to be either RocksDB or > >> InMemory, > >> > > > > based on > >> > > > > >>>>>> what > >> > > > > >>>>>>> is returned for the #persistent > >> > > > > >>>>>>> API by the StreamJoined's WindowStoreSupplier > >> > > > > >>>>>>> > >> > > > > >>>>>>> Does that sound right? We can clarify this further in the > >> KIP > >> > > if > >> > > > > need > >> > > > > >>>>>> be > >> > > > > >>>>>>> > >> > > > > >>>>>>> On Thu, Jul 27, 2023 at 10:48 AM Guozhang Wang < > >> > > > > >>>>>> guozhang.wang...@gmail.com> > >> > > > > >>>>>>> wrote: > >> > > > > >>>>>>> > >> > > > > >>>>>>>> Hi all, > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> Like Almog's secretary as well! Just following up on that > >> > > index: > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> A2: I'm also happy without introducing versioned KV in > >> this > >> > > KIP > >> > > > > as I > >> > > > > >>>>>>>> would envision it to be introduced as new params into the > >> > > > > >>>>>>>> KeyValuePluginParams in the future. And just to clarify > >> on > >> > > > > Sophie's > >> > > > > >>>>>>>> previous comment, I think ListStore should not be > >> exposed in > >> > > this > >> > > > > >> API > >> > > > > >>>>>>>> until we see it as a common usage and hence would want to > >> > > (again, > >> > > > > we > >> > > > > >>>>>>>> need to think very carefully since it would potentially > >> ask > >> > > all > >> > > > > >>>>>>>> implementers to adopt) move it from the internal > >> category to > >> > > the > >> > > > > >>>>>>>> public interface category. As for now, I think only > >> having kv > >> > > / > >> > > > > >>>>>> window > >> > > > > >>>>>>>> / session as public store types is fine. > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> A3: Seems I was not making myself very clear at the > >> beginning > >> > > :P > >> > > > > The > >> > > > > >>>>>>>> major thing that I'd actually like to avoid having two > >> configs > >> > > > > >>>>>>>> co-exist for the same function since it will be a > >> confusing > >> > > > > learning > >> > > > > >>>>>>>> curve for users, and hence what I was proposing is to > >> just > >> > > have > >> > > > > the > >> > > > > >>>>>>>> newly introduced interface but not introducing a new > >> config, > >> > > and I > >> > > > > >>>>>>>> realized now that it is actually more aligned with the > >> CUSTOM > >> > > idea > >> > > > > >>>>>>>> where the ordering would be looking at config first, and > >> then > >> > > the > >> > > > > >>>>>>>> interface. I blushed as I read Almog likes it.. After > >> thinking > >> > > > > about > >> > > > > >>>>>>>> it twice, I'm now a bit leaning towards just deprecating > >> the > >> > > old > >> > > > > >>>>>>>> config with the new API+config as well. > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> A5: Among the names we have been discussed so far: > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> DslStorePlugin > >> > > > > >>>>>>>> StoreTypeSpec > >> > > > > >>>>>>>> StoreImplSpec > >> > > > > >>>>>>>> DslStoreSuppliers > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> I am in favor of DslStoreSuppliers as well as a > >> > > restrictiveness on > >> > > > > >>>>>> its > >> > > > > >>>>>>>> scope, just to echo Bruno's comments above. > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> Guozhang > >> > > > > >>>>>>>> > >> > > > > >>>>>>>> On Thu, Jul 27, 2023 at 4:15 AM Bruno Cadonna < > >> > > cado...@apache.org > >> > > > > > > >> > > > > >>>>>> wrote: > >> > > > > >>>>>>>>> > >> > > > > >>>>>>>>> Hi, > >> > > > > >>>>>>>>> > >> > > > > >>>>>>>>> A5. I have to admit that > >> > > > > >>>>>>>>> "If we envision extending this beyond just StoreSupplier > >> > > types, > >> > > > > it > >> > > > > >>>>>> could > >> > > > > >>>>>>>>> be a good option." > >> > > > > >>>>>>>>> is scaring me a bit. > >> > > > > >>>>>>>>> I am wondering what would be an example for such an > >> > > extension? > >> > > > > >>>>>>>>> In general, I would propose to limit the scope of a > >> config. > >> > > In > >> > > > > >>>>>> this case > >> > > > > >>>>>>>>> the config should provide suppliers for state stores > >> for the > >> > > DSL. > >> > > > > >>>>>>>>> > >> > > > > >>>>>>>>> BTW, maybe it is a good idea to let DslStorePlugin > >> extend > >> > > > > >>>>>> Configurable. > >> > > > > >>>>>>>>> > >> > > > > >>>>>>>>> Best, > >> > > > > >>>>>>>>> Bruno > >> > > > > >>>>>>>>> > >> > > > > >>>>>>>>> On 7/27/23 2:15 AM, Sophie Blee-Goldman wrote: > >> > > > > >>>>>>>>>> Thanks for the feedback Bruno -- sounds like we're > >> getting > >> > > close > >> > > > > >>>>>> to a > >> > > > > >>>>>>>> final > >> > > > > >>>>>>>>>> consensus here. > >> > > > > >>>>>>>>>> It sounds like the two main (only?) semi-unresolved > >> > > questions > >> > > > > >>>>>> that > >> > > > > >>>>>>>> still > >> > > > > >>>>>>>>>> have differing > >> > > > > >>>>>>>>>> opinions floating around are whether to deprecate the > >> old > >> > > > > >>>>>> config, and > >> > > > > >>>>>>>> what > >> > > > > >>>>>>>>>> to name the new config > >> > > > > >>>>>>>>>> + interface. > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> Although I won't personally push back on any of the > >> options > >> > > > > >>>>>> listed > >> > > > > >>>>>>>> above, > >> > > > > >>>>>>>>>> here's my final two cents: > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> A3. I'm still a firm believer in deprecating the old > >> > > config, and > >> > > > > >>>>>> agree > >> > > > > >>>>>>>>>> wholeheartedly with what Bruno said. > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> A5. I also wasn't crazy about "Plugin" at first, but I > >> will > >> > > > > >>>>>> admit it's > >> > > > > >>>>>>>>>> grown on me. I think it rubbed me the wrong > >> > > > > >>>>>>>>>> way at first because it's just not part of the > >> standard > >> > > > > >>>>>> vocabulary in > >> > > > > >>>>>>>>>> Streams so far. If we envision extending > >> > > > > >>>>>>>>>> this beyond just StoreSupplier types, it could be a > >> good > >> > > option. > >> > > > > >>>>>>>>>> DSLStoreSuppliers does make a lot of sense, > >> > > > > >>>>>>>>>> though. > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> To throw out a few more ideas in case any of them > >> stick, > >> > > what > >> > > > > >>>>>> about > >> > > > > >>>>>>>>>> something like DSLStoreFormat or > >> > > > > >>>>>>>>>> DSLStorageType, or even DSLStorageEngine? Or even > >> > > > > >>>>>> DSLStoreFactory -- > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>> Stores class is described as > >> > > > > >>>>>>>>>> a "factory" (though not named so) and, to me, is > >> actually > >> > > quite > >> > > > > >>>>>>>> comparable > >> > > > > >>>>>>>>>> -- both are providers not of the > >> > > > > >>>>>>>>>> stores themselves, but of the basic building blocks of > >> > > Stores > >> > > > > (eg > >> > > > > >>>>>>>>>> StoreSuppliers) > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> Ultimately fine with anything though. We should try > >> not to > >> > > drag > >> > > > > >>>>>> out > >> > > > > >>>>>>>> the KIP > >> > > > > >>>>>>>>>> discussion too long once it's down > >> > > > > >>>>>>>>>> to just nits :P > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> Cheers, > >> > > > > >>>>>>>>>> Sophie > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>> On Wed, Jul 26, 2023 at 8:04 AM Almog Gavra < > >> > > > > >>>>>> almog.ga...@gmail.com> > >> > > > > >>>>>>>> wrote: > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>>>>> Thanks for the comments Bruno! > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>> A3. Oops... I think I didn't do a great job updating > >> the > >> > > KIP to > >> > > > > >>>>>>>> reflect > >> > > > > >>>>>>>>>>> Guozhang's suggestion. This seems like the last point > >> of > >> > > > > >>>>>> contention, > >> > > > > >>>>>>>> where > >> > > > > >>>>>>>>>>> we have two options: > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>> 1. Deprecate the config entirely and replace > >> > > IN_MEMORY/ROCKSDB > >> > > > > >>>>>> with > >> > > > > >>>>>>>>>>> implementations of the DslStorePlugin > >> > > > > >>>>>>>>>>> 2. (What's currently in the KIP) Introduce a new > >> config > >> > > which > >> > > > > >>>>>>>> defaults to > >> > > > > >>>>>>>>>>> DefaultDslStorePlugin and only the > >> DefaultDslStorePlugin > >> > > will > >> > > > > >>>>>> respect > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>> old default.store.type config > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>> I'm happy with either, I'll keep the KIP with (2) for > >> now > >> > > as > >> > > > > >>>>>> that > >> > > > > >>>>>>>> seemed > >> > > > > >>>>>>>>>>> like the result of the previous discussion but I have > >> no > >> > > > > problem > >> > > > > >>>>>>>> changing > >> > > > > >>>>>>>>>>> it back to (1) which was the original proposal. > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>> A5. I like "DslStorePlugin" because it leaves room for > >> > > > > >>>>>> configuring > >> > > > > >>>>>>>>>>> implementations beyond just supplying stores (e.g. we > >> could > >> > > > > >>>>>> introduce > >> > > > > >>>>>>>> a > >> > > > > >>>>>>>>>>> `configure()` method etc...). I'll keep it as is for > >> now > >> > > (and > >> > > > > >>>>>> change > >> > > > > >>>>>>>>>>> Materialized/Stores API sections - thanks for catching > >> > > that)! I > >> > > > > >>>>>> don't > >> > > > > >>>>>>>> feel > >> > > > > >>>>>>>>>>> too strongly and wouldn't dig my heels in if most > >> people > >> > > > > >>>>>> preferred > >> > > > > >>>>>>>>>>> "DslStoreSuppliers" (I don't love DslStores as it > >> > > resembles the > >> > > > > >>>>>> Stores > >> > > > > >>>>>>>>>>> class to closely in name and they're a little > >> different). > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>> A6. Yup, that's the suggestion. > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>> - Almog > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>> On Wed, Jul 26, 2023 at 6:38 AM Bruno Cadonna < > >> > > > > >>>>>> cado...@apache.org> > >> > > > > >>>>>>>> wrote: > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>>>> Hi, > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> Sorry for being late to the party! > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> A1: I agree with Sophie, Guozhang, and Almog not to > >> block > >> > > the > >> > > > > >>>>>> KIP on > >> > > > > >>>>>>>>>>>> gaps in the implementation. > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> A2: I am happy with not considering anything special > >> > > w.r.t. > >> > > > > >>>>>> versioned > >> > > > > >>>>>>>>>>>> state stores in this KIP. > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> A3: Here I agree with Sophie to deprecate the old > >> config. > >> > > I > >> > > > > >>>>>> would > >> > > > > >>>>>>>> also > >> > > > > >>>>>>>>>>>> not use config value CUSTOM. Having two configs that > >> > > sometimes > >> > > > > >>>>>>>> depend on > >> > > > > >>>>>>>>>>>> each other to configure one single concept seems > >> > > confusing to > >> > > > > >>>>>> me. I > >> > > > > >>>>>>>> see > >> > > > > >>>>>>>>>>>> future me looking at default.dsl.store = IN_MEMORY > >> and > >> > > > > >>>>>> wondering why > >> > > > > >>>>>>>>>>>> something is written to disk because I did not check > >> > > config > >> > > > > >>>>>>>>>>>> dsl.store.plugin.class? > >> > > > > >>>>>>>>>>>> BTW, the KIP in its current version is not clear > >> about > >> > > whether > >> > > > > >>>>>>>>>>>> default.dsl.store will be deprecated or not. In > >> > > > > "Compatibility, > >> > > > > >>>>>>>>>>>> Deprecation, and Migration Plan" it says > >> default.dsl.store > >> > > > > >>>>>> will be > >> > > > > >>>>>>>>>>>> deprecated but in "Configuration" default.dsl.store > >> seems > >> > > to > >> > > > > >>>>>> be an > >> > > > > >>>>>>>>>>>> essential part of the configuration. > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> A4: I agree > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> A5: I do not completely like the name > >> "DslStorePlugin". > >> > > What > >> > > > > >>>>>> about > >> > > > > >>>>>>>>>>>> naming it simply "DslStores" or "DslStoreSuppliers"? > >> If we > >> > > > > >>>>>> decide to > >> > > > > >>>>>>>>>>>> rename we should also rename dsl.store.plugin.class > >> to > >> > > > > >>>>>>>>>>>> dsl.store.suppliers.class or similar. > >> > > > > >>>>>>>>>>>> BTW, I think you missed to rename some occurrences in > >> > > section > >> > > > > >>>>>>>>>>>> "Materialized API" especially in the code section > >> > > > > >>>>>> "Stores.java". > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> A6: Actually I am not sure if I completely follow > >> here. Is > >> > > > > >>>>>> this about > >> > > > > >>>>>>>>>>>> the static methods in class Stores? If yes, I agree > >> with > >> > > Almog > >> > > > > >>>>>> to > >> > > > > >>>>>>>> keep > >> > > > > >>>>>>>>>>>> this out of the KIP. > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> Best, > >> > > > > >>>>>>>>>>>> Bruno > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> On 7/26/23 5:20 AM, Almog Gavra wrote: > >> > > > > >>>>>>>>>>>>> I have updated the KIP with the points as discussed > >> > > above. > >> > > > > >>>>>>>> @Guozhang, > >> > > > > >>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>> suggested configuration makes it a little more > >> awkward > >> > > around > >> > > > > >>>>>> the > >> > > > > >>>>>>>>>>>>> Materialized.as and Materialized.withStoreType APIs > >> than > >> > > it > >> > > > > >>>>>> was > >> > > > > >>>>>>>> when we > >> > > > > >>>>>>>>>>>>> were totally deprecating the old configuration. Let > >> me > >> > > know > >> > > > > >>>>>> what you > >> > > > > >>>>>>>>>>>> think. > >> > > > > >>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>> I will open the voting tomorrow! Thanks again > >> everyone > >> > > for > >> > > > > the > >> > > > > >>>>>>>>>>>> discussion. > >> > > > > >>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>> Cheers, > >> > > > > >>>>>>>>>>>>> Almog > >> > > > > >>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>> On Tue, Jul 25, 2023 at 9:20 AM Almog Gavra < > >> > > > > >>>>>> almog.ga...@gmail.com> > >> > > > > >>>>>>>>>>>> wrote: > >> > > > > >>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>> Glad you like my KIP-secretary skills ;) > >> > > > > >>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>> A2. I'm definitely happy to take your suggestion > >> here > >> > > and > >> > > > > >>>>>> not do > >> > > > > >>>>>>>>>>>> anything > >> > > > > >>>>>>>>>>>>>> special w.r.t. Versioned stores, I think it makes > >> sense > >> > > > > >>>>>> especially > >> > > > > >>>>>>>> if > >> > > > > >>>>>>>>>>> we > >> > > > > >>>>>>>>>>>>>> consider them implementation details of a specific > >> store > >> > > > > >>>>>> type. > >> > > > > >>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>> At EOD I'll update the KIP with all of these > >> changes > >> > > and if > >> > > > > >>>>>> the > >> > > > > >>>>>>>>>>>>>> discussion is silent I'll open a vote tomorrow > >> morning. > >> > > > > >>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>> Cheers, > >> > > > > >>>>>>>>>>>>>> Almog > >> > > > > >>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>> On Mon, Jul 24, 2023 at 2:02 PM Sophie > >> Blee-Goldman < > >> > > > > >>>>>>>>>>>>>> ableegold...@gmail.com> wrote: > >> > > > > >>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> Awesome summary (seriously) -- would you kindly > >> offer > >> > > your > >> > > > > >>>>>>>>>>>> organizational > >> > > > > >>>>>>>>>>>>>>> skills to every ongoing KIP from henceforth? We > >> need > >> > > you :P > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> A few answers/comments: > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> A2: I think there is a 3rd sub-option here, which > >> is to > >> > > > > >>>>>> leave > >> > > > > >>>>>>>>>>>>>>> versioned-ness out of this KIP entirely, return > >> only > >> > > the > >> > > > > >>>>>>>>>>> non-versioned > >> > > > > >>>>>>>>>>>>>>> stores for now, and then switch over to the > >> versioned > >> > > > > stores > >> > > > > >>>>>>>> (only) > >> > > > > >>>>>>>>>>>> when > >> > > > > >>>>>>>>>>>>>>> the time comes to flip the switch on making them > >> the > >> > > > > default > >> > > > > >>>>>>>> across > >> > > > > >>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>> DSL. This has the advantage of retaining the > >> current > >> > > > > >>>>>>>>>>> behavior/semantics > >> > > > > >>>>>>>>>>>>>>> and > >> > > > > >>>>>>>>>>>>>>> provides a clear way to transition smoothly in the > >> > > future, > >> > > > > >>>>>> since > >> > > > > >>>>>>>> it > >> > > > > >>>>>>>>>>>> seems > >> > > > > >>>>>>>>>>>>>>> we will want to cut to all versioned state stores > >> > > rather > >> > > > > >>>>>> than > >> > > > > >>>>>>>> offer > >> > > > > >>>>>>>>>>>> users > >> > > > > >>>>>>>>>>>>>>> a > >> > > > > >>>>>>>>>>>>>>> choice between versioned or non-versioned stores > >> going > >> > > > > >>>>>> forward > >> > > > > >>>>>>>>>>>> (similar to > >> > > > > >>>>>>>>>>>>>>> how we only offer timestamped stores presently, > >> and > >> > > have > >> > > > > >>>>>>>> completely > >> > > > > >>>>>>>>>>>>>>> replaced non-timestamped stores in the DSL.) . In > >> both > >> > > the > >> > > > > >>>>>>>>>>> timestamped > >> > > > > >>>>>>>>>>>> and > >> > > > > >>>>>>>>>>>>>>> versioned cases, the old stores are/will still be > >> > > available > >> > > > > >>>>>> or > >> > > > > >>>>>>>>>>>> accessible > >> > > > > >>>>>>>>>>>>>>> to users via the bare StoreSuppliers, should they > >> > > somehow > >> > > > > >>>>>> desire > >> > > > > >>>>>>>> or > >> > > > > >>>>>>>>>>>>>>> require > >> > > > > >>>>>>>>>>>>>>> the old store type. Ultimately, I think either > >> this or > >> > > > > >>>>>> option (1) > >> > > > > >>>>>>>>>>>> would be > >> > > > > >>>>>>>>>>>>>>> preferable, though I think it should be up to > >> Matthias > >> > > or > >> > > > > >>>>>> anyone > >> > > > > >>>>>>>> else > >> > > > > >>>>>>>>>>>>>>> involved in the versioned stores feature to decide > >> > > which > >> > > > > >>>>>> approach > >> > > > > >>>>>>>>>>> makes > >> > > > > >>>>>>>>>>>>>>> sense in the context of that feature's future > >> plans. > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> A3: sounds reasonable to me > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> A5: Also sounds fine to me, though I'll let others > >> > > chime in > >> > > > > >>>>>>>> with/if > >> > > > > >>>>>>>>>>>> they > >> > > > > >>>>>>>>>>>>>>> have an alternative suggestion/preference. I > >> guess the > >> > > > > other > >> > > > > >>>>>>>>>>> contender > >> > > > > >>>>>>>>>>>>>>> would be something like DSLStoreImpl or something > >> like > >> > > > > that? > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> On Mon, Jul 24, 2023 at 9:36 AM Almog Gavra < > >> > > > > >>>>>>>> almog.ga...@gmail.com> > >> > > > > >>>>>>>>>>>>>>> wrote: > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> Lots of thoughts! Happy to see the thriving > >> > > discussion on > >> > > > > >>>>>> this > >> > > > > >>>>>>>> post > >> > > > > >>>>>>>>>>> - > >> > > > > >>>>>>>>>>>>>>> lots > >> > > > > >>>>>>>>>>>>>>>> going on so I'm trying to enumerate them to keep > >> > > things > >> > > > > >>>>>> organized > >> > > > > >>>>>>>>>>>>>>> (prefix > >> > > > > >>>>>>>>>>>>>>>> "A" for "Almog" so we can use numbers in > >> responses for > >> > > > > >>>>>> other > >> > > > > >>>>>>>> things > >> > > > > >>>>>>>>>>>> ;P). > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A1. Question around closing implementation gaps > >> (e.g. > >> > > no > >> > > > > >>>>>> rocks > >> > > > > >>>>>>>> based > >> > > > > >>>>>>>>>>>>>>>> suppression store) > >> > > > > >>>>>>>>>>>>>>>> A2. Specifically how to handle Versioned stores > >> > > > > >>>>>>>>>>>>>>>> A3. Configuration (new config/reuse old one + > >> new one > >> > > and > >> > > > > >>>>>>>> ordering > >> > > > > >>>>>>>>>>> of > >> > > > > >>>>>>>>>>>>>>>> config resolution) > >> > > > > >>>>>>>>>>>>>>>> A4. Drawing a line between what is implementation > >> > > detail > >> > > > > >>>>>> (not > >> > > > > >>>>>>>>>>> exposed > >> > > > > >>>>>>>>>>>> in > >> > > > > >>>>>>>>>>>>>>>> API) and what is customizable (exposed in API) > >> > > > > >>>>>>>>>>>>>>>> A5. Naming of StoreTypeSpec > >> > > > > >>>>>>>>>>>>>>>> A6. Param classes in StoreBuilders > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> ------------------------------ > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> Here are summaries for where it seems each of > >> these > >> > > stands > >> > > > > >>>>>>>> (trying > >> > > > > >>>>>>>>>>> not > >> > > > > >>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>> add any additional opinion yet): > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A1. Sophie/Guozhang/Me (if I count hah!) seem to > >> agree > >> > > > > >>>>>> that it is > >> > > > > >>>>>>>>>>>> worth > >> > > > > >>>>>>>>>>>>>>>> pushing this KIP through independently of the > >> > > > > >>>>>> implementation > >> > > > > >>>>>>>> gaps as > >> > > > > >>>>>>>>>>>> it > >> > > > > >>>>>>>>>>>>>>>> doesn't seem to move the intermediate state > >> further > >> > > from > >> > > > > >>>>>> the end > >> > > > > >>>>>>>>>>>> state. > >> > > > > >>>>>>>>>>>>>>>> Matthias originally had some concerns. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A2. There's questions around whether versioned > >> stores > >> > > > > >>>>>> should be > >> > > > > >>>>>>>>>>> their > >> > > > > >>>>>>>>>>>>>>> own > >> > > > > >>>>>>>>>>>>>>>> configurable option or whether they are an > >> > > implementation > >> > > > > >>>>>> detail > >> > > > > >>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>> StoreSpec should decide. It seems like the > >> discussion > >> > > is > >> > > > > >>>>>>>> converging > >> > > > > >>>>>>>>>>>>>>> here, > >> > > > > >>>>>>>>>>>>>>>> this should be an implementation detail. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A3. Matthias/Guozhang prefer adding CUSTOM and > >> then > >> > > having > >> > > > > >>>>>> an > >> > > > > >>>>>>>>>>>> additional > >> > > > > >>>>>>>>>>>>>>>> config to determine the implementation. Sophie > >> prefers > >> > > > > >>>>>>>> deprecating > >> > > > > >>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>> old > >> > > > > >>>>>>>>>>>>>>>> config. Guozhang additional suggested flipping > >> the > >> > > > > >>>>>> resolution > >> > > > > >>>>>>>> order > >> > > > > >>>>>>>>>>>> such > >> > > > > >>>>>>>>>>>>>>>> that the old config is only respected in a > >> > > > > DefaultStoreSpec > >> > > > > >>>>>>>>>>>>>>> implementation. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A4. This KIP (or rather, the discussion on the > >> KIP) > >> > > blurs > >> > > > > >>>>>> the > >> > > > > >>>>>>>> lines > >> > > > > >>>>>>>>>>>>>>> between > >> > > > > >>>>>>>>>>>>>>>> top level store types (KV, windowed, session) > >> and the > >> > > > > >>>>>> underlying > >> > > > > >>>>>>>>>>>>>>>> implementation of them (timestamped, versioned, > >> > > kv-list). > >> > > > > >>>>>> It > >> > > > > >>>>>>>> seems > >> > > > > >>>>>>>>>>>>>>> everyone > >> > > > > >>>>>>>>>>>>>>>> is in alignment to ensure that we keep these two > >> > > things > >> > > > > >>>>>> separate > >> > > > > >>>>>>>> and > >> > > > > >>>>>>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>>> the line is clearly delineated in the text of > >> the KIP. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A5. Guozhang and Sophie agree that the current > >> name > >> > > > > >>>>>>>> StoreTypeSpec is > >> > > > > >>>>>>>>>>>>>>>> misleading, as it's really an implementation > >> spec, > >> > > not a > >> > > > > >>>>>> type > >> > > > > >>>>>>>>>>>>>>>> specification. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A6. Agreement that this is an improvement, Sophie > >> > > believes > >> > > > > >>>>>> this > >> > > > > >>>>>>>> can > >> > > > > >>>>>>>>>>> be > >> > > > > >>>>>>>>>>>>>>> done > >> > > > > >>>>>>>>>>>>>>>> in a follow up but we should ensure our naming > >> is good > >> > > > > >>>>>> here so > >> > > > > >>>>>>>>>>> there's > >> > > > > >>>>>>>>>>>>>>> no > >> > > > > >>>>>>>>>>>>>>>> conflicts down the line. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> --------------------------------- > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> Ok, phew! Hopefully that covers it all! Now for > >> my > >> > > > > >>>>>> thoughts, > >> > > > > >>>>>>>>>>> hopefully > >> > > > > >>>>>>>>>>>>>>>> wrapping up some of these discussions: > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A1. @Matthias - are you still hesitant here? > >> What > >> > > would > >> > > > > >>>>>> you > >> > > > > >>>>>>>> need to > >> > > > > >>>>>>>>>>>> be > >> > > > > >>>>>>>>>>>>>>>> convinced here? > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A2. Since we are all in agreement that versioned > >> > > stores > >> > > > > >>>>>> should > >> > > > > >>>>>>>> be an > >> > > > > >>>>>>>>>>>>>>>> implementation detail, we have two options: > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> (1) we can extend the KVParams to include a > >> parameter > >> > > that > >> > > > > >>>>>>>> indicates > >> > > > > >>>>>>>>>>>>>>>> whether or not the store should be versioned > >> > > > > >>>>>>>>>>>>>>>> (2) we can introduce a configuration for whether > >> or > >> > > not to > >> > > > > >>>>>> use a > >> > > > > >>>>>>>>>>>>>>> versioned > >> > > > > >>>>>>>>>>>>>>>> store, and each implementation can choose to > >> > > read/ignore > >> > > > > >>>>>> that > >> > > > > >>>>>>>> config > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> Any preferences? (1) would align more closely > >> with > >> > > what we > >> > > > > >>>>>> are > >> > > > > >>>>>>>> doing > >> > > > > >>>>>>>>>>>>>>> today > >> > > > > >>>>>>>>>>>>>>>> (they are a top-level concept in the Stores API). > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A3. I like Guozhang's suggestion of making the > >> > > "primary" > >> > > > > >>>>>>>>>>> configuration > >> > > > > >>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>> be the new one, and then having a > >> > > "DefaultStoreTypeSpec" > >> > > > > >>>>>> (using > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>> old > >> > > > > >>>>>>>>>>>>>>>> naming) which respects the old configuration. > >> That > >> > > seems > >> > > > > >>>>>> to solve > >> > > > > >>>>>>>>>>>> nearly > >> > > > > >>>>>>>>>>>>>>>> all the concerns (e.g. it'd be easy to see where > >> the > >> > > enum > >> > > > > >>>>>> is used > >> > > > > >>>>>>>>>>>>>>> because > >> > > > > >>>>>>>>>>>>>>>> it would only be used within that one class > >> instead of > >> > > > > >>>>>> littered > >> > > > > >>>>>>>>>>>>>>> throughout > >> > > > > >>>>>>>>>>>>>>>> the code base). > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> @Sophie, unless you have objections here I will > >> > > update the > >> > > > > >>>>>> KIP > >> > > > > >>>>>>>> to do > >> > > > > >>>>>>>>>>>>>>> that. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A4. I will make these changes to the KIP to make > >> it > >> > > clear. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A5. I will rename it `DslStorePlugin` - any > >> > > objections to > >> > > > > >>>>>> this > >> > > > > >>>>>>>> name? > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> A6. Let's punt this ;) I agree with everyone > >> that this > >> > > > > >>>>>> would be a > >> > > > > >>>>>>>>>>>>>>> welcome > >> > > > > >>>>>>>>>>>>>>>> improvement and that this KIP is aligned with > >> moving > >> > > in > >> > > > > >>>>>> that > >> > > > > >>>>>>>>>>>> direction. > >> > > > > >>>>>>>>>>>>>>>> Given how much discussion there was on this KIP, > >> > > which is > >> > > > > >>>>>> minor > >> > > > > >>>>>>>>>>>>>>> relative to > >> > > > > >>>>>>>>>>>>>>>> making the changes to StoreBuilder API, I'd > >> rather > >> > > not tie > >> > > > > >>>>>> the > >> > > > > >>>>>>>> two > >> > > > > >>>>>>>>>>>>>>>> together. > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> Cheers & Thanks everyone for the thoughts! > >> > > > > >>>>>>>>>>>>>>>> - Almog > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> On Sun, Jul 23, 2023 at 5:15 PM Sophie > >> Blee-Goldman < > >> > > > > >>>>>>>>>>>>>>>> ableegold...@gmail.com> > >> > > > > >>>>>>>>>>>>>>>> wrote: > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> Guozhang: > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> On your 2nd point: > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> "impl types" (in hindsight it may not be a good > >> > > name) > >> > > > > for > >> > > > > >>>>>>>> rocksdb > >> > > > > >>>>>>>>>>> / > >> > > > > >>>>>>>>>>>>>>>>> memory / custom, and we used "store types" for > >> kv / > >> > > > > >>>>>> windowed / > >> > > > > >>>>>>>>>>>>>>> sessioned > >> > > > > >>>>>>>>>>>>>>>>> etc, > >> > > > > >>>>>>>>>>>>>>>>> First off, thanks so much for this > >> clarification -- > >> > > using > >> > > > > >>>>>> "store > >> > > > > >>>>>>>>>>>> type" > >> > > > > >>>>>>>>>>>>>>>> here > >> > > > > >>>>>>>>>>>>>>>>> was definitely making me uncomfortable as this > >> > > usually > >> > > > > >>>>>> refers > >> > > > > >>>>>>>> to KV > >> > > > > >>>>>>>>>>>> vs > >> > > > > >>>>>>>>>>>>>>>>> window, etc -- but I just couldn't for the life > >> of me > >> > > > > >>>>>> think of > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>> right > >> > > > > >>>>>>>>>>>>>>>>> term for rocks vs IM. We should 100% change to > >> > > something > >> > > > > >>>>>> like > >> > > > > >>>>>>>>>>>>>>>> StoreImplSpec > >> > > > > >>>>>>>>>>>>>>>>> for this kind of interface. > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> As for list-value store (for stream-stream > >> Join) > >> > > > > >>>>>>>>>>>>>>>>> Again, glad you mentioned this -- I forgot how > >> the > >> > > extra > >> > > > > >>>>>>>>>>>> stream-stream > >> > > > > >>>>>>>>>>>>>>>> join > >> > > > > >>>>>>>>>>>>>>>>> store is not a "regular" KV Store but rather > >> this > >> > > special > >> > > > > >>>>>>>>>>> list-value > >> > > > > >>>>>>>>>>>>>>>> store. > >> > > > > >>>>>>>>>>>>>>>>> If we proceed with something like the current > >> > > approach, > >> > > > > >>>>>> perhaps > >> > > > > >>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>>> should > >> > > > > >>>>>>>>>>>>>>>>> be a boolean (or enum) parameter in the > >> KVConfig, > >> > > similar > >> > > > > >>>>>> to the > >> > > > > >>>>>>>>>>>>>>>>> EmitStrategy? After all, the high-level goal of > >> this > >> > > KIP > >> > > > > >>>>>> is to > >> > > > > >>>>>>>> be > >> > > > > >>>>>>>>>>>>>>> able to > >> > > > > >>>>>>>>>>>>>>>>> fully customize all DSL state stores, and this > >> is > >> > > > > >>>>>> currently not > >> > > > > >>>>>>>>>>>>>>> possible > >> > > > > >>>>>>>>>>>>>>>>> due to KAFKA-14976 < > >> > > > > >>>>>>>>>>>> https://issues.apache.org/jira/browse/KAFKA-14976 > >> > > > > >>>>>>>>>>>>>>>> . > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> If we expect there to be further customizations > >> like > >> > > this > >> > > > > >>>>>> going > >> > > > > >>>>>>>>>>>>>>> forward, > >> > > > > >>>>>>>>>>>>>>>>> perhaps we could instead have each of the three > >> > > > > >>>>>> StoreConfig > >> > > > > >>>>>>>> classes > >> > > > > >>>>>>>>>>>>>>>> accept > >> > > > > >>>>>>>>>>>>>>>>> a single enum parameter for the "sub-type" (or > >> > > whatever > >> > > > > >>>>>> you > >> > > > > >>>>>>>> want to > >> > > > > >>>>>>>>>>>>>>> call > >> > > > > >>>>>>>>>>>>>>>>> it), which would encompass (and replace) things > >> like > >> > > the > >> > > > > >>>>>>>>>>> EmitStrategy > >> > > > > >>>>>>>>>>>>>>> as > >> > > > > >>>>>>>>>>>>>>>>> well as the list-value type (we could define one > >> > > enum for > >> > > > > >>>>>> each > >> > > > > >>>>>>>>>>> Config > >> > > > > >>>>>>>>>>>>>>>> class > >> > > > > >>>>>>>>>>>>>>>>> so there is no accidentally requesting a > >> LIST_VALUE > >> > > > > >>>>>> subtype on a > >> > > > > >>>>>>>>>>>>>>>>> WindowStore). Thoughts? > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> Lastly, regarding 3.b: > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> I love that you brought this up because that is > >> > > actually > >> > > > > >>>>>> what I > >> > > > > >>>>>>>>>>> first > >> > > > > >>>>>>>>>>>>>>>>> proposed to Almog, ie introducing a param class > >> to > >> > > clean > >> > > > > >>>>>> up the > >> > > > > >>>>>>>>>>>>>>>>> StoreBuilder API, during our chat that led to > >> this > >> > > KIP. > >> > > > > He > >> > > > > >>>>>>>> pushed > >> > > > > >>>>>>>>>>>>>>> back, > >> > > > > >>>>>>>>>>>>>>>>> claiming (rightly so) that this change would be > >> huge > >> > > in > >> > > > > >>>>>> scope > >> > > > > >>>>>>>> for a > >> > > > > >>>>>>>>>>>>>>>> purely > >> > > > > >>>>>>>>>>>>>>>>> aesthetic/API change that doesn't add any > >> > > functionality, > >> > > > > >>>>>> and > >> > > > > >>>>>>>> that > >> > > > > >>>>>>>>>>> it > >> > > > > >>>>>>>>>>>>>>>> makes > >> > > > > >>>>>>>>>>>>>>>>> more sense to start with the DSL config since > >> there > >> > > is a > >> > > > > >>>>>> clear > >> > > > > >>>>>>>> gap > >> > > > > >>>>>>>>>>> in > >> > > > > >>>>>>>>>>>>>>>>> functionality there, particularly when it comes > >> to > >> > > custom > >> > > > > >>>>>> state > >> > > > > >>>>>>>>>>>> stores > >> > > > > >>>>>>>>>>>>>>>>> (reasons 1 & 3 in the Motivation section). He > >> did > >> > > agree > >> > > > > >>>>>> that the > >> > > > > >>>>>>>>>>>> param > >> > > > > >>>>>>>>>>>>>>>>> classes were a better API, which is why you see > >> them > >> > > in > >> > > > > >>>>>> this > >> > > > > >>>>>>>> KIP. > >> > > > > >>>>>>>>>>> In > >> > > > > >>>>>>>>>>>>>>>> other > >> > > > > >>>>>>>>>>>>>>>>> words: I fully agree that we should apply this > >> > > > > >>>>>> improvement to > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>> PAPI/StoreBuilder interface as well, but I think > >> > > that's a > >> > > > > >>>>>>>> distinct > >> > > > > >>>>>>>>>>>>>>>> concept > >> > > > > >>>>>>>>>>>>>>>>> for the time-being and should not block the DSL > >> > > > > >>>>>> improvement. > >> > > > > >>>>>>>>>>> Rather, > >> > > > > >>>>>>>>>>>> I > >> > > > > >>>>>>>>>>>>>>>>> consider this KIP as setting the stage for a > >> > > followup KIP > >> > > > > >>>>>> down > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>> line > >> > > > > >>>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>>> clean up the StoreBuilders and bring them in > >> line > >> > > with > >> > > > > >>>>>> the new > >> > > > > >>>>>>>>>>>>>>>> param/config > >> > > > > >>>>>>>>>>>>>>>>> class approach. > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> That said, A) we should definitely make sure > >> > > whatever we > >> > > > > >>>>>>>> introduce > >> > > > > >>>>>>>>>>>>>>> here > >> > > > > >>>>>>>>>>>>>>>>> can be extended to the PAPI StoreBuilders in a > >> > > natural > >> > > > > >>>>>> way, and > >> > > > > >>>>>>>> B) > >> > > > > >>>>>>>>>>> I > >> > > > > >>>>>>>>>>>>>>>> should > >> > > > > >>>>>>>>>>>>>>>>> clarify that I personally would be happy to see > >> this > >> > > > > >>>>>> included in > >> > > > > >>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>> current KIP, but as Almog's KIP it's up to him > >> to > >> > > decide > >> > > > > >>>>>> whether > >> > > > > >>>>>>>>>>> he's > >> > > > > >>>>>>>>>>>>>>>>> comfortable expanding the scope like this. If > >> you can > >> > > > > >>>>>> convince > >> > > > > >>>>>>>> him > >> > > > > >>>>>>>>>>>>>>> where > >> > > > > >>>>>>>>>>>>>>>> I > >> > > > > >>>>>>>>>>>>>>>>> could not, more power to you! :P > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> On Sun, Jul 23, 2023 at 4:48 PM Sophie > >> Blee-Goldman < > >> > > > > >>>>>>>>>>>>>>>>> ableegold...@gmail.com> > >> > > > > >>>>>>>>>>>>>>>>> wrote: > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> Matthias: > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> I'm not sure I agree with (or maybe don't > >> follow) > >> > > this > >> > > > > >>>>>> take: > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> we need all kind of `StoreTypeSpec` > >> > > implementations, > >> > > > > >>>>>>>>>>>>>>>>>>> and it might also imply that we need follow up > >> > > KIPs for > >> > > > > >>>>>> new > >> > > > > >>>>>>>>>>> feature > >> > > > > >>>>>>>>>>>>>>>>>>> (like in-memory versioned store) that might > >> not > >> > > need a > >> > > > > >>>>>> KIP > >> > > > > >>>>>>>>>>>>>>> otherwise. > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> I see this feature as being a nice > >> > > add-on/convenience > >> > > > > >>>>>> API for > >> > > > > >>>>>>>> any > >> > > > > >>>>>>>>>>>>>>> store > >> > > > > >>>>>>>>>>>>>>>>>> types which have a full DSL implementation. I > >> don't > >> > > > > >>>>>> think it's > >> > > > > >>>>>>>>>>>>>>>>> unreasonable > >> > > > > >>>>>>>>>>>>>>>>>> to just say that this feature is only going to > >> be > >> > > > > >>>>>> available for > >> > > > > >>>>>>>>>>>>>>> store > >> > > > > >>>>>>>>>>>>>>>>> types > >> > > > > >>>>>>>>>>>>>>>>>> that have KV, Window, and Session > >> implementations. I > >> > > > > >>>>>> can't > >> > > > > >>>>>>>> think > >> > > > > >>>>>>>>>>> of > >> > > > > >>>>>>>>>>>>>>> any > >> > > > > >>>>>>>>>>>>>>>>>> case besides versioned stores where this would > >> > > force a > >> > > > > >>>>>> KIP for > >> > > > > >>>>>>>> a > >> > > > > >>>>>>>>>>> new > >> > > > > >>>>>>>>>>>>>>>>>> feature that would not otherwise have to go > >> through > >> > > a > >> > > > > >>>>>> KIP, and > >> > > > > >>>>>>>>>>> even > >> > > > > >>>>>>>>>>>>>>> for > >> > > > > >>>>>>>>>>>>>>>>>> versioned state stores, the only issue is that > >> the > >> > > KIP > >> > > > > >>>>>> for that > >> > > > > >>>>>>>>>>> was > >> > > > > >>>>>>>>>>>>>>>>> already > >> > > > > >>>>>>>>>>>>>>>>>> accepted. > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> However, I think I agree on your main point -- > >> that > >> > > > > >>>>>> things like > >> > > > > >>>>>>>>>>>>>>>> "regular" > >> > > > > >>>>>>>>>>>>>>>>>> vs timestamped vs versioned are/should be an > >> > > > > >>>>>> implementation > >> > > > > >>>>>>>> detail > >> > > > > >>>>>>>>>>>>>>>> that's > >> > > > > >>>>>>>>>>>>>>>>>> hidden from the user. As I noted previously, > >> the > >> > > current > >> > > > > >>>>>> KIP > >> > > > > >>>>>>>>>>>>>>> actually > >> > > > > >>>>>>>>>>>>>>>>>> greatly improves the situation for timestamped > >> > > stores, > >> > > > > >>>>>> as this > >> > > > > >>>>>>>>>>>>>>> would be > >> > > > > >>>>>>>>>>>>>>>>>> handled completely transparently by the OOTB > >> > > > > >>>>>> RocksDBStoreSpec. > >> > > > > >>>>>>>> To > >> > > > > >>>>>>>>>>>>>>> me, > >> > > > > >>>>>>>>>>>>>>>>> this > >> > > > > >>>>>>>>>>>>>>>>>> provides a very natural way to let the DSL > >> operators > >> > > > > >>>>>> using the > >> > > > > >>>>>>>>>>>>>>> default > >> > > > > >>>>>>>>>>>>>>>>>> store type/spec to specify which kind of store > >> (eg > >> > > > > >>>>>>>>>>>>>>>>>> versioned/timestamped/etc) it wants, and > >> choose the > >> > > > > >>>>>> correct > >> > > > > >>>>>>>>>>>>>>> default. If > >> > > > > >>>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>> eventual intention is to have versioned state > >> stores > >> > > > > >>>>>> replace > >> > > > > >>>>>>>>>>>>>>>> timestamped > >> > > > > >>>>>>>>>>>>>>>>>> stores as the default in the DSL, then we can > >> simply > >> > > > > >>>>>> swap out > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>> versioned > >> > > > > >>>>>>>>>>>>>>>>>> stores for the timestamped stores in the > >> > > > > >>>>>> RocksDBStoreTypeSpec, > >> > > > > >>>>>>>>>>> when > >> > > > > >>>>>>>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>>>>> time comes. Until then, users who want to use > >> the > >> > > > > >>>>>> versioned > >> > > > > >>>>>>>> store > >> > > > > >>>>>>>>>>>>>>> will > >> > > > > >>>>>>>>>>>>>>>>> have > >> > > > > >>>>>>>>>>>>>>>>>> to do what they do today, which is individually > >> > > override > >> > > > > >>>>>>>> operators > >> > > > > >>>>>>>>>>>>>>> via > >> > > > > >>>>>>>>>>>>>>>>>> Materialized/StoreSuppliers. > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> All in all, it sounds like we should not offer > >> a > >> > > > > >>>>>> versioned > >> > > > > >>>>>>>> store > >> > > > > >>>>>>>>>>>>>>> type > >> > > > > >>>>>>>>>>>>>>>>>> spec, as "versioned" is more akin to > >> "timestamped" > >> > > than > >> > > > > >>>>>> to a > >> > > > > >>>>>>>> true > >> > > > > >>>>>>>>>>>>>>>>>> difference in underlying store implementation > >> type > >> > > (eg > >> > > > > >>>>>> rocks vs > >> > > > > >>>>>>>>>>>>>>>>> in-memory). > >> > > > > >>>>>>>>>>>>>>>>>> W.r.t whether to deprecate the old config or > >> > > introduce a > >> > > > > >>>>>> new > >> > > > > >>>>>>>>>>> CUSTOM > >> > > > > >>>>>>>>>>>>>>>> enum > >> > > > > >>>>>>>>>>>>>>>>>> type, either seems fine to me, and we can go > >> with > >> > > that > >> > > > > >>>>>>>> alternative > >> > > > > >>>>>>>>>>>>>>>>> instead. > >> > > > > >>>>>>>>>>>>>>>>>> The only other con to this approach that I can > >> > > think of, > >> > > > > >>>>>> and > >> > > > > >>>>>>>> I'm > >> > > > > >>>>>>>>>>>>>>>> honestly > >> > > > > >>>>>>>>>>>>>>>>>> not sure if this is something users would care > >> > > about or > >> > > > > >>>>>> only > >> > > > > >>>>>>>> devs, > >> > > > > >>>>>>>>>>>>>>> is > >> > > > > >>>>>>>>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>>>>> the advantage to moving rocks and IM to the > >> store > >> > > type > >> > > > > >>>>>> spec > >> > > > > >>>>>>>>>>>>>>> interface > >> > > > > >>>>>>>>>>>>>>>> is > >> > > > > >>>>>>>>>>>>>>>>>> that it helps to keep the relevant logic > >> > > encapsulated in > >> > > > > >>>>>> one > >> > > > > >>>>>>>> easy > >> > > > > >>>>>>>>>>>>>>> place > >> > > > > >>>>>>>>>>>>>>>>> you > >> > > > > >>>>>>>>>>>>>>>>>> can quickly check to tell what kind of state > >> store > >> > > is > >> > > > > >>>>>> used > >> > > > > >>>>>>>> where. > >> > > > > >>>>>>>>>>> In > >> > > > > >>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>> current code, I found it extremely annoying and > >> > > > > >>>>>> difficult to > >> > > > > >>>>>>>> track > >> > > > > >>>>>>>>>>>>>>> down > >> > > > > >>>>>>>>>>>>>>>>> all > >> > > > > >>>>>>>>>>>>>>>>>> usages of the StoreType enum to see which > >> actual > >> > > rocksdb > >> > > > > >>>>>> store > >> > > > > >>>>>>>> was > >> > > > > >>>>>>>>>>>>>>>> being > >> > > > > >>>>>>>>>>>>>>>>>> used where (for example some stores using the > >> > > > > >>>>>> TimeOrderedBuffer > >> > > > > >>>>>>>>>>>>>>>> variants > >> > > > > >>>>>>>>>>>>>>>>> in > >> > > > > >>>>>>>>>>>>>>>>>> some special cases, or to understand whether > >> the > >> > > DSL was > >> > > > > >>>>>>>>>>> defaulting > >> > > > > >>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>>>> plain, timestamped, or versioned stores for > >> RocksDB > >> > > vs > >> > > > > >>>>>>>> InMemory -- > >> > > > > >>>>>>>>>>>>>>> both > >> > > > > >>>>>>>>>>>>>>>>> of > >> > > > > >>>>>>>>>>>>>>>>>> which seem like they could be of interest to a > >> > > user). > >> > > > > >>>>>> This > >> > > > > >>>>>>>> would > >> > > > > >>>>>>>>>>> be > >> > > > > >>>>>>>>>>>>>>>> much > >> > > > > >>>>>>>>>>>>>>>>>> easier if everything was handled in one place, > >> and > >> > > you > >> > > > > >>>>>> can > >> > > > > >>>>>>>> just go > >> > > > > >>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>> (eg) RocksDBStoreTypeSpec and see what it's > >> doing, > >> > > or > >> > > > > >>>>>> find > >> > > > > >>>>>>>> usages > >> > > > > >>>>>>>>>>> of > >> > > > > >>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>> methods to understand what stores are being > >> handed > >> > > to > >> > > > > >>>>>> which DSL > >> > > > > >>>>>>>>>>>>>>>>> operators. > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> I suppose we could still clean up the API and > >> solve > >> > > this > >> > > > > >>>>>>>> problem > >> > > > > >>>>>>>>>>> by > >> > > > > >>>>>>>>>>>>>>>>> having > >> > > > > >>>>>>>>>>>>>>>>>> the old (and new) config delegate to a > >> > > StoreTypeSpec no > >> > > > > >>>>>> matter > >> > > > > >>>>>>>>>>> what, > >> > > > > >>>>>>>>>>>>>>>> but > >> > > > > >>>>>>>>>>>>>>>>>> make RocksDBStoreTypeSpec and > >> InMemoryStoreTypeSpec > >> > > > > >>>>>> internal > >> > > > > >>>>>>>>>>> classes > >> > > > > >>>>>>>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>>>>> are simply implementation details of the > >> ROCKSDB vs > >> > > > > >>>>>> IN_MEMORY > >> > > > > >>>>>>>>>>> enums. > >> > > > > >>>>>>>>>>>>>>>>> WDYT? > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> On Sun, Jul 23, 2023 at 11:14 AM Guozhang Wang > >> < > >> > > > > >>>>>>>>>>>>>>>>> guozhang.wang...@gmail.com> > >> > > > > >>>>>>>>>>>>>>>>>> wrote: > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> Thanks everyone for the great discussions so > >> far! I > >> > > > > >>>>>> first saw > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>> JIRA > >> > > > > >>>>>>>>>>>>>>>>>>> and left some quick thoughts without being > >> aware > >> > > of the > >> > > > > >>>>>>>>>>>>>>>>>>> already-written KIP (kudos to Almog, very > >> great > >> > > one) > >> > > > > >>>>>> and the > >> > > > > >>>>>>>>>>>>>>> DISCUSS > >> > > > > >>>>>>>>>>>>>>>>>>> thread here. And I happily find some of my > >> initial > >> > > > > >>>>>> thoughts > >> > > > > >>>>>>>> align > >> > > > > >>>>>>>>>>>>>>> with > >> > > > > >>>>>>>>>>>>>>>>>>> the KIP already :) > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> Would like to add a bit more of my 2c after > >> reading > >> > > > > >>>>>> through > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>> KIP > >> > > > > >>>>>>>>>>>>>>>>>>> and the thread here: > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> 1. On the high level, I'm in favor of pushing > >> this > >> > > KIP > >> > > > > >>>>>> through > >> > > > > >>>>>>>>>>>>>>> without > >> > > > > >>>>>>>>>>>>>>>>>>> waiting on the other gaps to be closed. In my > >> back > >> > > > > >>>>>> pocket's > >> > > > > >>>>>>>>>>>>>>>>>>> "dependency graph" of Kafka Streams roadmap of > >> > > large > >> > > > > >>>>>> changes > >> > > > > >>>>>>>> or > >> > > > > >>>>>>>>>>>>>>>>>>> feature gaps, the edges of dependencies are > >> defined > >> > > > > >>>>>> based on > >> > > > > >>>>>>>> my > >> > > > > >>>>>>>>>>>>>>>>>>> understanding of whether doing one first would > >> > > largely > >> > > > > >>>>>>>>>>> complicate / > >> > > > > >>>>>>>>>>>>>>>>>>> negate the effort of the other but not vice > >> versa, > >> > > in > >> > > > > >>>>>> which > >> > > > > >>>>>>>> case > >> > > > > >>>>>>>>>>> we > >> > > > > >>>>>>>>>>>>>>>>>>> should consider getting the other done first. > >> In > >> > > this > >> > > > > >>>>>> case, I > >> > > > > >>>>>>>>>>> feel > >> > > > > >>>>>>>>>>>>>>>>>>> such a dependency is not strong enough, so > >> > > encouraging > >> > > > > >>>>>> the KIP > >> > > > > >>>>>>>>>>>>>>>>>>> contributor to finish what he/she would love > >> to do > >> > > to > >> > > > > >>>>>> close > >> > > > > >>>>>>>> some > >> > > > > >>>>>>>>>>>>>>> gaps > >> > > > > >>>>>>>>>>>>>>>>>>> early would be higher priorities. I did not > >> see by > >> > > just > >> > > > > >>>>>> doing > >> > > > > >>>>>>>>>>> this > >> > > > > >>>>>>>>>>>>>>> we > >> > > > > >>>>>>>>>>>>>>>>>>> could end up in a worse intermediate stage > >> yet, > >> > > but I > >> > > > > >>>>>> could be > >> > > > > >>>>>>>>>>>>>>>>>>> corrected. > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> 2. Regarding the store types --- gain here I'd > >> > > like to > >> > > > > >>>>>> just > >> > > > > >>>>>>>>>>> clarify > >> > > > > >>>>>>>>>>>>>>>>>>> the terms a bit since in the past it has some > >> > > > > >>>>>> confusions: we > >> > > > > >>>>>>>> used > >> > > > > >>>>>>>>>>>>>>>>>>> "impl types" (in hindsight it may not be a > >> good > >> > > name) > >> > > > > >>>>>> for > >> > > > > >>>>>>>>>>> rocksdb / > >> > > > > >>>>>>>>>>>>>>>>>>> memory / custom, and we used "store types" > >> for kv / > >> > > > > >>>>>> windowed / > >> > > > > >>>>>>>>>>>>>>>>>>> sessioned etc, as I said in the JIRA I think > >> the > >> > > > > current > >> > > > > >>>>>>>> proposal > >> > > > > >>>>>>>>>>>>>>> also > >> > > > > >>>>>>>>>>>>>>>>>>> have a good side effect as quality bar to > >> really > >> > > > > >>>>>> enforce us > >> > > > > >>>>>>>> think > >> > > > > >>>>>>>>>>>>>>>>>>> twice when trying to add more store types in > >> the > >> > > future > >> > > > > >>>>>> as it > >> > > > > >>>>>>>>>>> will > >> > > > > >>>>>>>>>>>>>>>>>>> impact API instantiations. In the ideal > >> world, I > >> > > would > >> > > > > >>>>>>>> consider: > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> * We have (timestamped) kv store, versioned kv > >> > > store, > >> > > > > >>>>>> window > >> > > > > >>>>>>>>>>> store, > >> > > > > >>>>>>>>>>>>>>>>>>> session store as first-class DSL store types. > >> Some > >> > > DSL > >> > > > > >>>>>>>> operators > >> > > > > >>>>>>>>>>>>>>> could > >> > > > > >>>>>>>>>>>>>>>>>>> accept multiple store types (e.g. versioned > >> and non > >> > > > > >>>>>> versioned > >> > > > > >>>>>>>>>>>>>>>>>>> kv-store) for semantics / efficiency > >> trade-offs. > >> > > But I > >> > > > > >>>>>> think > >> > > > > >>>>>>>> we > >> > > > > >>>>>>>>>>>>>>> would > >> > > > > >>>>>>>>>>>>>>>>>>> remove un-timestamped kv stores eventually > >> since > >> > > that > >> > > > > >>>>>>>> efficiency > >> > > > > >>>>>>>>>>>>>>>>>>> trade-off is so minimal compared to its usage > >> > > > > >>>>>> limitations. > >> > > > > >>>>>>>>>>>>>>>>>>> * As for list-value store (for stream-stream > >> Join), > >> > > > > >>>>>>>>>>>>>>> memory-lru-cache > >> > > > > >>>>>>>>>>>>>>>>>>> (for PAPI use only), > >> memory-time-ordered-buffer > >> > > (for > >> > > > > >>>>>>>>>>> suppression), > >> > > > > >>>>>>>>>>>>>>>>>>> they would not be exposed as DSL first-class > >> store > >> > > > > >>>>>> types in > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> future. Instead, they would be treated as > >> internal > >> > > used > >> > > > > >>>>>> stores > >> > > > > >>>>>>>>>>>>>>> (e.g. > >> > > > > >>>>>>>>>>>>>>>>>>> list-value store is built on key-value store > >> with > >> > > > > >>>>>> specialized > >> > > > > >>>>>>>>>>> serde > >> > > > > >>>>>>>>>>>>>>>>>>> and putInternal), or continue to be just > >> convenient > >> > > > > >>>>>> OOTB PAPI > >> > > > > >>>>>>>>>>> used > >> > > > > >>>>>>>>>>>>>>>>>>> stores only. > >> > > > > >>>>>>>>>>>>>>>>>>> * As we move on, we will continue to be very, > >> very > >> > > > > >>>>>> strict on > >> > > > > >>>>>>>> what > >> > > > > >>>>>>>>>>>>>>>>>>> would be added as DSL store types (and hence > >> > > requires > >> > > > > >>>>>> changes > >> > > > > >>>>>>>> to > >> > > > > >>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> proposed APIs), what to be added as > >> convenient OOTB > >> > > > > >>>>>> PAPI store > >> > > > > >>>>>>>>>>>>>>> impls > >> > > > > >>>>>>>>>>>>>>>>>>> only, what to be added as internal used store > >> types > >> > > > > that > >> > > > > >>>>>>>> should > >> > > > > >>>>>>>>>>>>>>> not be > >> > > > > >>>>>>>>>>>>>>>>>>> exposed to users nor customizable at all. > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> 3. Some more detailed thoughts below: > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> 3.a) I originally also think that we can > >> extend the > >> > > > > >>>>>> existing > >> > > > > >>>>>>>>>>>>>>> config, > >> > > > > >>>>>>>>>>>>>>>>>>> rather than replacing it. The difference was > >> that > >> > > I was > >> > > > > >>>>>>>> thinking > >> > > > > >>>>>>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>>>>>> order-wise, the runtime would look at the API > >> > > first, > >> > > > > >>>>>> and then > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> config, whereas in your rejected alternative > >> it was > >> > > > > >>>>>> looking at > >> > > > > >>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> config first, and then the API --- that I > >> think is > >> > > a > >> > > > > >>>>>> minor > >> > > > > >>>>>>>> thing > >> > > > > >>>>>>>>>>>>>>> and > >> > > > > >>>>>>>>>>>>>>>>>>> either is fine. I'm in agreement that having > >> two > >> > > > > configs > >> > > > > >>>>>>>> would be > >> > > > > >>>>>>>>>>>>>>> more > >> > > > > >>>>>>>>>>>>>>>>>>> confusing to users to learn about their > >> precedence > >> > > > > >>>>>> rather than > >> > > > > >>>>>>>>>>>>>>>>>>> helpful, but if we keep both a config and a > >> public > >> > > API, > >> > > > > >>>>>> then > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> precedence ordering would not be so confusing > >> as > >> > > long > >> > > > > >>>>>> as we > >> > > > > >>>>>>>> state > >> > > > > >>>>>>>>>>>>>>> them > >> > > > > >>>>>>>>>>>>>>>>>>> clearly. For example: > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> * We have DefaultStoreTypeSpec OOTB, in that > >> impl > >> > > we > >> > > > > >>>>>> look at > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> config only, and would only expect either > >> ROCKS or > >> > > > > >>>>>> MEMORY, and > >> > > > > >>>>>>>>>>>>>>> return > >> > > > > >>>>>>>>>>>>>>>>>>> corresponding OOTB store impls; if any other > >> values > >> > > > > >>>>>>>> configured, > >> > > > > >>>>>>>>>>> we > >> > > > > >>>>>>>>>>>>>>>>>>> error out. > >> > > > > >>>>>>>>>>>>>>>>>>> * Users extend that by having > >> MyStoreTypeSpec, in > >> > > which > >> > > > > >>>>>> they > >> > > > > >>>>>>>>>>> could > >> > > > > >>>>>>>>>>>>>>> do > >> > > > > >>>>>>>>>>>>>>>>>>> arbituray things without respecting the > >> config at > >> > > all, > >> > > > > >>>>>> but our > >> > > > > >>>>>>>>>>>>>>>>>>> recommended pattern in docs would still say > >> "look > >> > > into > >> > > > > >>>>>> the > >> > > > > >>>>>>>>>>> config, > >> > > > > >>>>>>>>>>>>>>> if > >> > > > > >>>>>>>>>>>>>>>>>>> it is ROCKS or MEMORY just return fall back to > >> > > > > >>>>>>>>>>>>>>> DefaultStoreTypeSepc; > >> > > > > >>>>>>>>>>>>>>>>>>> otherwise if it's some String you recognize, > >> then > >> > > > > >>>>>> return your > >> > > > > >>>>>>>>>>>>>>>>>>> customized store based on the string value, > >> > > otherwise > >> > > > > >>>>>> error > >> > > > > >>>>>>>> out". > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> 3.b) About the struct-like Params classes, I > >> like > >> > > the > >> > > > > >>>>>> idea a > >> > > > > >>>>>>>> lot > >> > > > > >>>>>>>>>>>>>>> and > >> > > > > >>>>>>>>>>>>>>>>>>> wished we would pursue this in the first > >> place, > >> > > but if > >> > > > > >>>>>> we > >> > > > > >>>>>>>> only do > >> > > > > >>>>>>>>>>>>>>> this > >> > > > > >>>>>>>>>>>>>>>>>>> in Spec it would leave some inconsistencies > >> with > >> > > the > >> > > > > >>>>>>>>>>> StoreBuilders > >> > > > > >>>>>>>>>>>>>>>>>>> though arguably the latter is only for PAPI. > >> I'm > >> > > > > >>>>>> wondering if > >> > > > > >>>>>>>> we > >> > > > > >>>>>>>>>>>>>>>>>>> should consider including the changes in > >> > > StoreBuilders > >> > > > > >>>>>> (e.g. > >> > > > > >>>>>>>>>>>>>>>>>>> WindowStoreBuilder(WindowSupplierParams)), > >> and if > >> > > yes, > >> > > > > >>>>>> maybe > >> > > > > >>>>>>>> we > >> > > > > >>>>>>>>>>>>>>> should > >> > > > > >>>>>>>>>>>>>>>>>>> also consider renaming that e.g. > >> > > `WindowSupplierParams` > >> > > > > >>>>>> to > >> > > > > >>>>>>>>>>>>>>>>>>> `WindowStoreSpecParams` too? For this one I > >> only > >> > > have a > >> > > > > >>>>>> "weak > >> > > > > >>>>>>>>>>>>>>> feeling" > >> > > > > >>>>>>>>>>>>>>>>>>> so I can be convinced otherwise :P > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> Thanks, > >> > > > > >>>>>>>>>>>>>>>>>>> Guozhang > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> On Sun, Jul 23, 2023 at 9:52 AM Matthias J. > >> Sax < > >> > > > > >>>>>>>>>>> mj...@apache.org> > >> > > > > >>>>>>>>>>>>>>>>> wrote: > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> Thanks for all the input. My intention was > >> not to > >> > > > > >>>>>> block the > >> > > > > >>>>>>>> KIP, > >> > > > > >>>>>>>>>>>>>>> but > >> > > > > >>>>>>>>>>>>>>>>>>>> just to take a step back and try get a > >> holistic > >> > > > > >>>>>> picture and > >> > > > > >>>>>>>>>>>>>>>>> discussion, > >> > > > > >>>>>>>>>>>>>>>>>>>> to explore if there are good/viable > >> alternative > >> > > > > >>>>>> designs. As > >> > > > > >>>>>>>> said > >> > > > > >>>>>>>>>>>>>>>>>>>> originally, I really like to close this gap, > >> and > >> > > was > >> > > > > >>>>>> always > >> > > > > >>>>>>>>>>> aware > >> > > > > >>>>>>>>>>>>>>>> that > >> > > > > >>>>>>>>>>>>>>>>>>>> the current config is not flexible enough. > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> I guess, my "concern" is that the KIP does > >> > > increase > >> > > > > >>>>>> the API > >> > > > > >>>>>>>>>>>>>>> surface > >> > > > > >>>>>>>>>>>>>>>>> area > >> > > > > >>>>>>>>>>>>>>>>>>>> significantly, as we need all kind of > >> > > `StoreTypeSpec` > >> > > > > >>>>>>>>>>>>>>>> implementations, > >> > > > > >>>>>>>>>>>>>>>>>>>> and it might also imply that we need follow > >> up > >> > > KIPs > >> > > > > >>>>>> for new > >> > > > > >>>>>>>>>>>>>>> feature > >> > > > > >>>>>>>>>>>>>>>>>>>> (like in-memory versioned store) that might > >> not > >> > > need a > >> > > > > >>>>>> KIP > >> > > > > >>>>>>>>>>>>>>>> otherwise. > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> The second question is if it might make the > >> > > already > >> > > > > >>>>>> "patchy" > >> > > > > >>>>>>>>>>>>>>>> situation > >> > > > > >>>>>>>>>>>>>>>>>>>> with regard to customization worse. > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> We did de-scope the original KIP-591 for this > >> > > reason, > >> > > > > >>>>>> and > >> > > > > >>>>>>>> given > >> > > > > >>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>> new > >> > > > > >>>>>>>>>>>>>>>>>>>> situation of the DSL, it seems that it > >> actually > >> > > got > >> > > > > >>>>>> worse > >> > > > > >>>>>>>>>>>>>>> compared > >> > > > > >>>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>>>>>> back in the days. > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> Lastly, I hope to make the new versioned > >> stores > >> > > the > >> > > > > >>>>>> default > >> > > > > >>>>>>>> in > >> > > > > >>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>> DSL > >> > > > > >>>>>>>>>>>>>>>>>>>> and we did not do it in the previous KIP due > >> to > >> > > > > >>>>>> backward > >> > > > > >>>>>>>>>>>>>>>> compatibility > >> > > > > >>>>>>>>>>>>>>>>>>>> issues. Thus, from a DSL point of view, I > >> believe > >> > > > > there > >> > > > > >>>>>>>> should > >> > > > > >>>>>>>>>>> be > >> > > > > >>>>>>>>>>>>>>>> only > >> > > > > >>>>>>>>>>>>>>>>>>>> "RocksDB", "InMemory", and "Custom" in an > >> ideal > >> > > world. > >> > > > > >>>>>>>>>>>>>>> Introducing > >> > > > > >>>>>>>>>>>>>>>> (I > >> > > > > >>>>>>>>>>>>>>>>> am > >> > > > > >>>>>>>>>>>>>>>>>>>> exaggerating to highlight my point) > >> > > "KvRocksDbSpec", > >> > > > > >>>>>>>>>>>>>>>>>>>> "TimestampeKvRocksDbSpec", > >> "VersionedRocksDbSpec", > >> > > > > >>>>>> plus the > >> > > > > >>>>>>>>>>>>>>>>>>>> corresponding in-memory specs seems to head > >> into > >> > > the > >> > > > > >>>>>> opposite > >> > > > > >>>>>>>>>>>>>>>>> direction. > >> > > > > >>>>>>>>>>>>>>>>>>>> -- My goal is to give users a handle of the > >> > > _physical_ > >> > > > > >>>>>> store > >> > > > > >>>>>>>>>>>>>>>> (RocksDB > >> > > > > >>>>>>>>>>>>>>>>> vs > >> > > > > >>>>>>>>>>>>>>>>>>>> InMemory vs Custom) but not the _logical_ > >> stores > >> > > > > >>>>>> (plain kv, > >> > > > > >>>>>>>>>>>>>>> ts-kv, > >> > > > > >>>>>>>>>>>>>>>>>>>> versioned) which is "dictated" by the DSL > >> itself > >> > > and > >> > > > > >>>>>> should > >> > > > > >>>>>>>> not > >> > > > > >>>>>>>>>>>>>>> be > >> > > > > >>>>>>>>>>>>>>>>>>>> customizable (we are just in a weird > >> intermediate > >> > > > > >>>>>> situation > >> > > > > >>>>>>>> that > >> > > > > >>>>>>>>>>>>>>> we > >> > > > > >>>>>>>>>>>>>>>>> need > >> > > > > >>>>>>>>>>>>>>>>>>>> to clean up, but not "lean into" IMHO). > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> Thus, I am also not sure if adding > >> > > > > >>>>>> "VersionedRocksDbSpec" > >> > > > > >>>>>>>> would > >> > > > > >>>>>>>>>>>>>>> be > >> > > > > >>>>>>>>>>>>>>>>> ideal > >> > > > > >>>>>>>>>>>>>>>>>>>> (also, given that it only changes a single > >> store, > >> > > but > >> > > > > >>>>>> not the > >> > > > > >>>>>>>>>>> two > >> > > > > >>>>>>>>>>>>>>>>>>>> windowed stores)? > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> Furthermore, I actually hope that we could > >> use > >> > > the new > >> > > > > >>>>>>>> versioned > >> > > > > >>>>>>>>>>>>>>>> store > >> > > > > >>>>>>>>>>>>>>>>>>>> to replace the window- and sessions- stores, > >> and > >> > > thus > >> > > > > >>>>>> to > >> > > > > >>>>>>>>>>> decrease > >> > > > > >>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>>> number of required store types. > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> Admittedly, I am talking a lot about a > >> potential > >> > > > > >>>>>> future, but > >> > > > > >>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>> goal > >> > > > > >>>>>>>>>>>>>>>>> is > >> > > > > >>>>>>>>>>>>>>>>>>>> only to explore opportunities to not get into > >> > > "worse" > >> > > > > >>>>>>>>>>>>>>> intermediate > >> > > > > >>>>>>>>>>>>>>>>>>>> state, that will require a huge deprecation > >> > > surface > >> > > > > >>>>>> area > >> > > > > >>>>>>>> later > >> > > > > >>>>>>>>>>>>>>> on. > >> > > > > >>>>>>>>>>>>>>>> Of > >> > > > > >>>>>>>>>>>>>>>>>>>> course, if there is no better way, and my > >> > > concerns are > >> > > > > >>>>>> not > >> > > > > >>>>>>>>>>>>>>> shared, I > >> > > > > >>>>>>>>>>>>>>>>> am > >> > > > > >>>>>>>>>>>>>>>>>>>> ok to move forward with the KIP. > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> Bottom line: I would personally prefer to > >> keep the > >> > > > > >>>>>> current > >> > > > > >>>>>>>>>>> config > >> > > > > >>>>>>>>>>>>>>>> and > >> > > > > >>>>>>>>>>>>>>>>>>>> add a `Custom` option to it, plus adding one > >> new > >> > > > > >>>>>> config that > >> > > > > >>>>>>>>>>>>>>> allows > >> > > > > >>>>>>>>>>>>>>>>>>>> people to set their custom `StoreTypeSpec` > >> class. > >> > > -- I > >> > > > > >>>>>> would > >> > > > > >>>>>>>> not > >> > > > > >>>>>>>>>>>>>>>> add a > >> > > > > >>>>>>>>>>>>>>>>>>>> built-in spec for versioned stores at this > >> point > >> > > (or > >> > > > > >>>>>> any > >> > > > > >>>>>>>> other > >> > > > > >>>>>>>>>>>>>>>>> built-in > >> > > > > >>>>>>>>>>>>>>>>>>>> `StorytypeSpec` implementations). I guess, > >> users > >> > > could > >> > > > > >>>>>> write > >> > > > > >>>>>>>> a > >> > > > > >>>>>>>>>>>>>>>> custom > >> > > > > >>>>>>>>>>>>>>>>>>>> spec if they want to enable versioned store > >> > > across the > >> > > > > >>>>>> board > >> > > > > >>>>>>>> for > >> > > > > >>>>>>>>>>>>>>> now > >> > > > > >>>>>>>>>>>>>>>>>>>> (until we make them the default anyway)? > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> Hope my train of thoughts is halfway > >> reasonable > >> > > and > >> > > > > not > >> > > > > >>>>>>>> totally > >> > > > > >>>>>>>>>>>>>>> off > >> > > > > >>>>>>>>>>>>>>>>>>> track? > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> -Matthias > >> > > > > >>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>> On 7/21/23 15:27, Sophie Blee-Goldman wrote: > >> > > > > >>>>>>>>>>>>>>>>>>>>> I agree with everything Almog said above, > >> and > >> > > will > >> > > > > >>>>>> just add > >> > > > > >>>>>>>> on > >> > > > > >>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>>> two > >> > > > > >>>>>>>>>>>>>>>>>>>>> points: > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> 1. Regarding whether to block this KIP on > >> the > >> > > > > >>>>>> completion of > >> > > > > >>>>>>>>>>>>>>> any or > >> > > > > >>>>>>>>>>>>>>>>> all > >> > > > > >>>>>>>>>>>>>>>>>>>>> future implementations of in-memory version > >> > > stores > >> > > > > (or > >> > > > > >>>>>>>> persist > >> > > > > >>>>>>>>>>>>>>>>>>> suppression > >> > > > > >>>>>>>>>>>>>>>>>>>>> buffers), I feel that would be unfair to > >> this > >> > > feature > >> > > > > >>>>>> which > >> > > > > >>>>>>>> is > >> > > > > >>>>>>>>>>>>>>>>>>> completely > >> > > > > >>>>>>>>>>>>>>>>>>>>> unrelated to the semantic improvements > >> offered by > >> > > > > >>>>>> versioned > >> > > > > >>>>>>>>>>>>>>> state > >> > > > > >>>>>>>>>>>>>>>>>>> stores. > >> > > > > >>>>>>>>>>>>>>>>>>>>> It seems like the responsibility of those > >> > > driving the > >> > > > > >>>>>>>> versioned > >> > > > > >>>>>>>>>>>>>>>>> state > >> > > > > >>>>>>>>>>>>>>>>>>>>> stores feature, not Almog/this KIP, to make > >> sure > >> > > that > >> > > > > >>>>>> those > >> > > > > >>>>>>>>>>>>>>> bases > >> > > > > >>>>>>>>>>>>>>>>> are > >> > > > > >>>>>>>>>>>>>>>>>>>>> covered. Further, if anything, this KIP > >> will help > >> > > > > >>>>>> with the > >> > > > > >>>>>>>>>>>>>>> massive > >> > > > > >>>>>>>>>>>>>>>>>>>>> proliferation of StoreSuppliers on the > >> Stores > >> > > factory > >> > > > > >>>>>> class, > >> > > > > >>>>>>>>>>>>>>> and > >> > > > > >>>>>>>>>>>>>>>>>>> provide > >> > > > > >>>>>>>>>>>>>>>>>>>>> users with a much easier way to leverage the > >> > > > > versioned > >> > > > > >>>>>>>> stores > >> > > > > >>>>>>>>>>>>>>>>> without > >> > > > > >>>>>>>>>>>>>>>>>>>>> having to muck around directly with the > >> > > > > >>>>>> StoreSuppliers. > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> I also thought about it a bit, and really > >> like > >> > > > > Almog's > >> > > > > >>>>>>>>>>>>>>> suggestion > >> > > > > >>>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>>>>>>> introduce an additional StoreSpec for the > >> > > Versioned > >> > > > > >>>>>> state > >> > > > > >>>>>>>>>>>>>>> stores. > >> > > > > >>>>>>>>>>>>>>>>>>> Obviously > >> > > > > >>>>>>>>>>>>>>>>>>>>> we can add the RocksDB one to this KIP now, > >> and > >> > > then > >> > > > > >>>>>> as he > >> > > > > >>>>>>>>>>>>>>>>> mentioned, > >> > > > > >>>>>>>>>>>>>>>>>>>>> there's an easy way to get users onto the > >> > > > > >>>>>>>> IMVersionedStateStore > >> > > > > >>>>>>>>>>>>>>>>> types > >> > > > > >>>>>>>>>>>>>>>>>>> once > >> > > > > >>>>>>>>>>>>>>>>>>>>> they are completed. > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> Lastly, on this note, I want to point out > >> how > >> > > > > >>>>>> smoothly this > >> > > > > >>>>>>>>>>>>>>> solved > >> > > > > >>>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>>>> issue of timestamped stores, which are > >> intended > >> > > to be > >> > > > > >>>>>> the > >> > > > > >>>>>>>> DSL > >> > > > > >>>>>>>>>>>>>>>>> default > >> > > > > >>>>>>>>>>>>>>>>>>> but > >> > > > > >>>>>>>>>>>>>>>>>>>>> are only a special case for RocksDB. Right > >> now > >> > > it can > >> > > > > >>>>>> be > >> > > > > >>>>>>>>>>>>>>> confusing > >> > > > > >>>>>>>>>>>>>>>>>>> for a > >> > > > > >>>>>>>>>>>>>>>>>>>>> user scrolling through the endless Stores > >> class > >> > > and > >> > > > > >>>>>> seeing a > >> > > > > >>>>>>>>>>>>>>>>>>> timestamped > >> > > > > >>>>>>>>>>>>>>>>>>>>> version of the persistent but not in-memory > >> > > stores. > >> > > > > >>>>>> One > >> > > > > >>>>>>>> could > >> > > > > >>>>>>>>>>>>>>>> easily > >> > > > > >>>>>>>>>>>>>>>>>>> assume > >> > > > > >>>>>>>>>>>>>>>>>>>>> there was no timestamped option for IM > >> stores and > >> > > > > >>>>>> that this > >> > > > > >>>>>>>>>>>>>>>> feature > >> > > > > >>>>>>>>>>>>>>>>>>> was > >> > > > > >>>>>>>>>>>>>>>>>>>>> incomplete, if they weren't acutely aware > >> of the > >> > > > > >>>>>> internal > >> > > > > >>>>>>>>>>>>>>>>>>> implementation > >> > > > > >>>>>>>>>>>>>>>>>>>>> details (namely that it's only required for > >> > > RocksDB > >> > > > > >>>>>> for > >> > > > > >>>>>>>>>>>>>>>>> compatibility > >> > > > > >>>>>>>>>>>>>>>>>>>>> reasons). However, with this KIP, all that > >> is > >> > > handled > >> > > > > >>>>>>>>>>>>>>> completely > >> > > > > >>>>>>>>>>>>>>>>>>>>> transparently to the user, and we the devs, > >> who > >> > > *are* > >> > > > > >>>>>> aware > >> > > > > >>>>>>>> of > >> > > > > >>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> internal > >> > > > > >>>>>>>>>>>>>>>>>>>>> implementation details, are now > >> appropriately the > >> > > > > ones > >> > > > > >>>>>>>>>>>>>>> responsible > >> > > > > >>>>>>>>>>>>>>>>> for > >> > > > > >>>>>>>>>>>>>>>>>>>>> handing the correct store type to a > >> particular > >> > > > > >>>>>> operator. > >> > > > > >>>>>>>> While > >> > > > > >>>>>>>>>>>>>>>>>>> versioned > >> > > > > >>>>>>>>>>>>>>>>>>>>> state stores may not be completely > >> comparable, > >> > > > > >>>>>> depending on > >> > > > > >>>>>>>>>>>>>>>> whether > >> > > > > >>>>>>>>>>>>>>>>>>> we want > >> > > > > >>>>>>>>>>>>>>>>>>>>> users to remain able to easily choose > >> between > >> > > using > >> > > > > >>>>>> them or > >> > > > > >>>>>>>> not > >> > > > > >>>>>>>>>>>>>>>> (vs > >> > > > > >>>>>>>>>>>>>>>>>>>>> timestamped which should be used by all), I > >> still > >> > > > > >>>>>> feel this > >> > > > > >>>>>>>> KIP > >> > > > > >>>>>>>>>>>>>>>> is a > >> > > > > >>>>>>>>>>>>>>>>>>> great > >> > > > > >>>>>>>>>>>>>>>>>>>>> step in the right direction that not only > >> should > >> > > not > >> > > > > >>>>>> be > >> > > > > >>>>>>>>>>>>>>> blocked on > >> > > > > >>>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>>>> completion of the IM implementations, but > >> in fact > >> > > > > >>>>>> should > >> > > > > >>>>>>>>>>>>>>>>> specifically > >> > > > > >>>>>>>>>>>>>>>>>>> be > >> > > > > >>>>>>>>>>>>>>>>>>>>> done first as it enables an easier way to > >> utilize > >> > > > > >>>>>> those IM > >> > > > > >>>>>>>>>>>>>>>> versioned > >> > > > > >>>>>>>>>>>>>>>>>>>>> stores. Just my 2 cents :) > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> 2. The idea to expand the existing the > >> config > >> > > with a > >> > > > > >>>>>> CUSTOM > >> > > > > >>>>>>>>>>>>>>> enum > >> > > > > >>>>>>>>>>>>>>>>>>> without > >> > > > > >>>>>>>>>>>>>>>>>>>>> introducing another config to specify the > >> CUSTOM > >> > > > > >>>>>> store spec > >> > > > > >>>>>>>>>>>>>>> does > >> > > > > >>>>>>>>>>>>>>>> not > >> > > > > >>>>>>>>>>>>>>>>>>> seem > >> > > > > >>>>>>>>>>>>>>>>>>>>> appropriate, or even possible (for the > >> reasons > >> > > Almog > >> > > > > >>>>>>>> mentioned > >> > > > > >>>>>>>>>>>>>>>>> above > >> > > > > >>>>>>>>>>>>>>>>>>> about > >> > > > > >>>>>>>>>>>>>>>>>>>>> config types, though perhaps there is a way > >> I'm > >> > > not > >> > > > > >>>>>>>> seeing). I > >> > > > > >>>>>>>>>>>>>>> do > >> > > > > >>>>>>>>>>>>>>>>> not > >> > > > > >>>>>>>>>>>>>>>>>>> buy > >> > > > > >>>>>>>>>>>>>>>>>>>>> the argument that we should optimize the > >> API to > >> > > make > >> > > > > >>>>>> it easy > >> > > > > >>>>>>>>>>>>>>> for > >> > > > > >>>>>>>>>>>>>>>>>>> users who > >> > > > > >>>>>>>>>>>>>>>>>>>>> just want to switch to all in-memory > >> stores, as I > >> > > > > >>>>>> truly > >> > > > > >>>>>>>> believe > >> > > > > >>>>>>>>>>>>>>>> this > >> > > > > >>>>>>>>>>>>>>>>>>> is a > >> > > > > >>>>>>>>>>>>>>>>>>>>> very small fraction of the potential > >> userbase of > >> > > this > >> > > > > >>>>>>>> feature > >> > > > > >>>>>>>>>>>>>>>>> (anyone > >> > > > > >>>>>>>>>>>>>>>>>>> who's > >> > > > > >>>>>>>>>>>>>>>>>>>>> actually using this should please chime > >> in!). It > >> > > > > >>>>>> seems very > >> > > > > >>>>>>>>>>>>>>> likely > >> > > > > >>>>>>>>>>>>>>>>>>> that the > >> > > > > >>>>>>>>>>>>>>>>>>>>> majority of users of this feature are > >> actually > >> > > those > >> > > > > >>>>>> with > >> > > > > >>>>>>>>>>>>>>> custom > >> > > > > >>>>>>>>>>>>>>>>> state > >> > > > > >>>>>>>>>>>>>>>>>>>>> stores, as to my knowledge, this has been > >> the > >> > > case > >> > > > > >>>>>> any/every > >> > > > > >>>>>>>>>>>>>>> time > >> > > > > >>>>>>>>>>>>>>>>> this > >> > > > > >>>>>>>>>>>>>>>>>>>>> feature was requested by users. > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> That said, while I don't see any way to get > >> > > around > >> > > > > >>>>>>>> introducing > >> > > > > >>>>>>>>>>>>>>> a > >> > > > > >>>>>>>>>>>>>>>> new > >> > > > > >>>>>>>>>>>>>>>>>>>>> config, I don't personally have a preference > >> > > w.r.t > >> > > > > >>>>>> whether > >> > > > > >>>>>>>> to > >> > > > > >>>>>>>>>>>>>>> keep > >> > > > > >>>>>>>>>>>>>>>>>>> around > >> > > > > >>>>>>>>>>>>>>>>>>>>> the old config or deprecate it. I simply > >> don't > >> > > get > >> > > > > the > >> > > > > >>>>>>>>>>>>>>> impression > >> > > > > >>>>>>>>>>>>>>>> it > >> > > > > >>>>>>>>>>>>>>>>>>> is > >> > > > > >>>>>>>>>>>>>>>>>>>>> very heavily used as it stands today, while > >> it > >> > > only > >> > > > > >>>>>> works > >> > > > > >>>>>>>> for > >> > > > > >>>>>>>>>>>>>>>> those > >> > > > > >>>>>>>>>>>>>>>>>>> who > >> > > > > >>>>>>>>>>>>>>>>>>>>> want all in-memory stores. Again, input from > >> > > actual > >> > > > > >>>>>> users > >> > > > > >>>>>>>>>>>>>>> would be > >> > > > > >>>>>>>>>>>>>>>>>>> very > >> > > > > >>>>>>>>>>>>>>>>>>>>> valuable here. In the absence of that data, > >> I > >> > > will > >> > > > > >>>>>> just > >> > > > > >>>>>>>> point > >> > > > > >>>>>>>>>>>>>>> to > >> > > > > >>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>> fact > >> > > > > >>>>>>>>>>>>>>>>>>>>> that this KIP was proposed by a Streams dev > >> (you > >> > > :P), > >> > > > > >>>>>>>>>>>>>>> abandoned, > >> > > > > >>>>>>>>>>>>>>>>>>> picked up > >> > > > > >>>>>>>>>>>>>>>>>>>>> by another Streams dev, and finally > >> implemented > >> > > > > >>>>>> without ever > >> > > > > >>>>>>>>>>>>>>>> hearing > >> > > > > >>>>>>>>>>>>>>>>>>> from a > >> > > > > >>>>>>>>>>>>>>>>>>>>> user that they would find this useful. This > >> is > >> > > not to > >> > > > > >>>>>>>> disparage > >> > > > > >>>>>>>>>>>>>>>> the > >> > > > > >>>>>>>>>>>>>>>>>>>>> original KIP, but just to say again, as I > >> stated > >> > > back > >> > > > > >>>>>> then, > >> > > > > >>>>>>>> it > >> > > > > >>>>>>>>>>>>>>>>> seemed > >> > > > > >>>>>>>>>>>>>>>>>>> like > >> > > > > >>>>>>>>>>>>>>>>>>>>> a major opportunity loss to leave out custom > >> > > state > >> > > > > >>>>>> stores > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> Cheers, > >> > > > > >>>>>>>>>>>>>>>>>>>>> Sophie > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> On Fri, Jul 21, 2023 at 1:52 PM Almog Gavra > >> < > >> > > > > >>>>>>>>>>>>>>>> almog.ga...@gmail.com> > >> > > > > >>>>>>>>>>>>>>>>>>> wrote: > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>>> 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 > >> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>>> > >> > > > > >>>>>>>>>>>> > >> > > > > >>>>>>>>>>> > >> > > > > >>>>>>>>>> > >> > > > > >>>>>>>> > >> > > > > >>>>>> > >> > > > > >>>>> > >> > > > > >>> > >> > > > > >> > >> > > > > > > >> > > > > > >> > > > >> > >
