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