Hi Matthias, Good point! I didn't notice that. Yes, I agree it's not good to let store depends on kstream package. I'll remove the stores API change later. (after Chinese new year :) )
Thanks for your good suggestion! Luke Matthias J. Sax <mj...@apache.org> 於 2022年2月1日 週二 下午3:10 寫道: > Thanks for updating the KIP. > > It's much clearer now. I think the improvement to `Materialized` is a > good one. > > However, I still have doubts about `Stores`. While the API change itself > seems ok (even if I don't see a large benefit), it adds a dependency > from the DSL classes (ie, package `kstream`) to the state store package > `state`. I think it would be better to avoid "cyclic" package > dependencies. While it does not matter atm, as it's all a single module, > we usually try to avoid "reverse" dependencies: neither the `processor` > nor `state` packages should use anything from the `kstream` package IMHO. > > > -Matthias > > > > On 1/30/22 12:00 AM, Luke Chen wrote: > > Hi John and Guozhang, > > > > Thanks for your comments. > > > > And @John, yes, you are right. > > The goal of the improved Materialized API is to provide a way to use > > im-memory store without providing the name. > > So, about this comment: > > > >> On the other hand, I don't see how the latter of these is > > more compelling than the former: > > .count(Materialized.as(Stores.inMemoryKeyValueStore("count- > > store"))); > > .count(Materialized.as(Stores.keyValueStoreSupplier(StoreTyp > > e.IN_MEMORY, "count-store"))); > > > > I think I didn't make it clear here. > > The improved Materialized API is like this > > > .count(Materialized.as(Materialized.withStoreType(StoreImplType.IN_MEMORY)); > > // without name provided. > > > > I've updated the KIP to make it clear. > > > > Therefore, I'll keep the Materialize/Stores change and complete the PR. > > > > Thanks for your comments again! > > > > Luke > > > > > > > > On Sat, Jan 29, 2022 at 7:46 AM John Roesler <vvcep...@apache.org> > wrote: > > > >> Hi Luke, > >> > >> Thanks for the KIP! > >> > >> I'm +1 (binding) on your KIP. > >> > >> Regarding this last question about chaning Materialized > >> and/or Stores, I think it might actually be best to drop > >> that part of the proposal. > >> > >> The primary benefit of your proposal is in the cases when > >> the user doesn't want to specify the store type at all and > >> just, as a blanket, use in-memory stores across the whole > >> topology instead of rocksDB ones. For that, we have the > >> config you proposed. > >> > >> As I read it, the Materialized part of the proposal was > >> secondary; to allow users to override the default storage > >> engine on a per-operation basis without having to bother > >> about providing a full-fledged store supplier. In other > >> words, today, if you want an in-memory store on a grouped > >> stream, you have to do: > >> > >> .count(Materialized.as(Stores.inMemoryKeyValueStore("count- > >> store"))); > >> > >> What if you didn't care about the name but wanted it to be > >> in memory? Well, you're out of luck. > >> > >> Therefore, I think there's significant value in modifying > >> the DSL to allow users to orthogonally specify the storage > >> engine and the name of the store, as in your KIP as written. > >> > >> On the other hand, I don't see how the latter of these is > >> more compelling than the former: > >> .count(Materialized.as(Stores.inMemoryKeyValueStore("count- > >> store"))); > >> .count(Materialized.as(Stores.keyValueStoreSupplier(StoreTyp > >> e.IN_MEMORY, "count-store"))); > >> > >> > >> Regardless, I don't want to let perfect be the enemy of > >> good. Like I said, I think that the key benefit you're > >> really going for is the config, so maybe you want to just > >> drop the Materialize/Stores aspect and simplify the > >> proposal. Or if you want to keep the latter, I'm fine with > >> whatever approach you feel is best (which is why I still > >> voted). > >> > >> This feels like the kind of thing that won't really be > >> crystal clear until the PR is under review (and I'd > >> encourage you and the reviewer to pay particular attention > >> to how the new APIs actually look when used in the tests). > >> > >> Thanks again! People have been asking for this for a long > >> time. > >> -John > >> > >> > >> On Fri, 2022-01-28 at 13:46 -0800, Guozhang Wang wrote: > >>> Hi Luke, > >>> > >>> I'm in favor of using the newly proposed `#sessionStore(StoreType..)` > and > >>> deprecating the existing `#persistenSessionStore` etc. > >>> > >>> Thanks, > >>> Guozhang > >>> > >>> On Tue, Jan 25, 2022 at 12:17 AM Luke Chen <show...@gmail.com> wrote: > >>> > >>>> Thanks Matthias! > >>>> > >>>> I agree we could deprecate the existing ones, and add the one with > >>>> storeType parameter. > >>>> > >>>> That is: > >>>> @deprecated > >>>> Stores#persistentSessionStore(...) > >>>> @deprecated > >>>> Stores#inMemorySessionStore(...) > >>>> @new added with an additional storeType parameter (IN_MEMORY or > >> ROCKS_DB) > >>>> Stores#sessionStoreSupplier(StoreType storeType, ...) > >>>> > >>>> Let's see what others think about it. > >>>> > >>>> Thank you. > >>>> Luke > >>>> > >>>> On Tue, Jan 25, 2022 at 4:01 PM Matthias J. Sax <mj...@apache.org> > >> wrote: > >>>> > >>>>> Thanks, > >>>>> > >>>>> There is already `Stores.persistentSessionStore` and > >>>>> `Stores.inMemorySessionStore`. From a DSL code POV, I don't see large > >>>>> benefits to add a new one, but it also does not hurt. > >>>>> > >>>>> Do you propose to add the third one only, or to also deprecate the > >>>>> existing ones? In general, we should avoid to extend the API surface > >>>>> area, so it could be a good simplification is we plan to remove the > >>>>> existing ones? > >>>>> > >>>>> Btw: we could name the new method just `sessionStoreSupplier` for > >>>>> simplicity (especially, if we deprecate the existing ones)? > >>>>> > >>>>> Not sure what others think. I am fine adding it, if we deprecate the > >>>>> existing ones. > >>>>> > >>>>> -Matthias > >>>>> > >>>>> > >>>>> On 1/24/22 5:03 PM, Luke Chen wrote: > >>>>>> Hi Matthias, > >>>>>> > >>>>>> I didn't "save" the change. >.< > >>>>>> Anyway, you can refer to this WIP PR to have better understanding > >>>>> why/what > >>>>>> the new API is: > >>>>>> > >>>>> > >>>> > >> > https://github.com/apache/kafka/pull/11705/files#diff-c552e58e01169886c5d8b8b149f5c8cd48ea1fc1c3d7b932d055d3df9a00e1b5R464-R477 > >>>>>> > >>>>>> It's not necessary, actually, but it can make the implementation > >>>> cleaner. > >>>>>> If you think this change is unnecessary and will make the `Stores` > >> API > >>>>> more > >>>>>> complicated, it's fine to me. > >>>>>> > >>>>>> I'll update the KIP after we have a conclusion for it. > >>>>>> > >>>>>> Thank you. > >>>>>> Luke > >>>>>> > >>>>>> On Tue, Jan 25, 2022 at 2:37 AM Matthias J. Sax <mj...@apache.org> > >>>>> wrote: > >>>>>> > >>>>>>> I don't see the KIP update? Did you hit "save"? > >>>>>>> > >>>>>>> Also, the formatting in your email for the new methods is hard to > >>>> read. > >>>>>>> Not sure atm why we need the API change? Can you elaborate? what > >> does > >>>>>>> > >>>>>>>> I found it'd be better > >>>>>>> > >>>>>>> > >>>>>>> -Matthias > >>>>>>> > >>>>>>> > >>>>>>> On 1/24/22 2:29 AM, Luke Chen wrote: > >>>>>>>> Thanks for all your votes. > >>>>>>>> > >>>>>>>> During the implementation, I found it'd be better to have > >> helper > >>>>> methods > >>>>>>> in > >>>>>>>> `Stores`, to be able to get the store supplier by the store > >> type: > >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> *public static SessionBytesStoreSupplier > >>>>>>>> sessionStoreSupplierByStoreType()public static > >>>> WindowBytesStoreSupplier > >>>>>>>> windowStoreSupplierByStoreType()public static > >>>>> KeyValueBytesStoreSupplier > >>>>>>>> keyValueStoreSupplierByStoreType()* > >>>>>>>> > >>>>>>>> I've also updated in the KIP. > >>>>>>>> Please let me know if you other thoughts. > >>>>>>>> > >>>>>>>> Also, welcome to vote for this KIP. > >>>>>>>> > >>>>>>>> Thank you. > >>>>>>>> Luke > >>>>>>>> > >>>>>>>> > >>>>>>>> On Fri, Jan 21, 2022 at 4:39 AM Walker Carlson > >>>>>>>> <wcarl...@confluent.io.invalid> wrote: > >>>>>>>> > >>>>>>>>> +1 non binding > >>>>>>>>> > >>>>>>>>> On Thu, Jan 20, 2022 at 2:00 PM Matthias J. Sax < > >> mj...@apache.org> > >>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> +1 (binding) > >>>>>>>>>> > >>>>>>>>>> On 1/20/22 10:52 AM, Guozhang Wang wrote: > >>>>>>>>>>> Thanks Luke! I'm +1 on the KIP. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> Guozhang > >>>>>>>>>>> > >>>>>>>>>>> On Wed, Jan 19, 2022 at 5:58 PM Luke Chen < > >> show...@gmail.com> > >>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>>> Hi devs, > >>>>>>>>>>>> > >>>>>>>>>>>> I'd like to start a vote for the KIP-591: Add Kafka > >> Streams > >>>> config > >>>>> to > >>>>>>>>>> set > >>>>>>>>>>>> default state store. The goal is to allow users to set > >> a default > >>>>>>> store > >>>>>>>>>> in > >>>>>>>>>>>> the config, so it can apply to all the streams. > >>>>>>>>>>>> > >>>>>>>>>>>> Detailed description can be found here: > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>> > >>>> > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-591%3A+Add+Kafka+Streams+config+to+set+default+state+store > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> Thank you. > >>>>>>>>>>>> Luke > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >>> > >> > >> > > >