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