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