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