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 >> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>>> >> > > > > >>>>>>>>>>>>> >> > > > > >>>>>>>>>>>> >> > > > > >>>>>>>>>>> >> > > > > >>>>>>>>>> >> > > > > >>>>>>>> >> > > > > >>>>>> >> > > > > >>>>> >> > > > > >>> >> > > > > >> >> > > > > > >> > > > > >> > > >> >
