Hi Matthias and all,
Thanks for your comments.
I've updated the KIP to remove the Stores API change.
And I'll also close the vote for this KIP.
The KIP is accepted with 3 binding votes (Matthias, Guozhang, and John).
Thank you very much.
Luke
On Tue, Feb 1, 2022 at 4:36 PM Luke Chen <show...@gmail.com> wrote:
> 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
>> >>>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>
>> >>>>>>>
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>
>> >>>
>> >>
>> >>
>> >
>>
>
