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