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