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