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