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