Hi Guozhang,
Thanks for the comments.
And I agree it's better to rename it to `default.dsl.store.impl.type` for
differentiation.
I've updated the KIP.
Thank you.
Luke
On Wed, Dec 22, 2021 at 3:12 AM Guozhang Wang <wangg...@gmail.com> wrote:
> Thanks Luke, I do not have any major comments on the wiki any more. BTW
> thanks for making the "public StreamsBuilder(final TopologyConfig
> topologyConfigs)" API public now, I think it will benefit a lot of future
> works!
>
> I think with the new API, we can deprecate the `build(props)` function in
> StreamsBuilder now, and will file a separate JIRA for it.
>
> Just a few nits:
>
> 1) There seems to be a typo at the end: "ROCK_DB"
> 2) Sometimes people refer to "store type" as kv-store, window-store etc;
> maybe we can differentiate them a bit by calling the new API names
> `StoreImplType`,
> `default.dsl.store.impl.type` and `The default store implementation type
> used by DSL operators`.
>
> On Thu, Dec 16, 2021 at 2:29 AM Luke Chen <show...@gmail.com> wrote:
>
> > Hi Guozhang,
> >
> > I've updated the KIP to use `enum`, instead of store implementation, and
> > some content accordingly.
> > Please let me know if there's other comments.
> >
> >
> > Thank you.
> > Luke
> >
> > On Sun, Dec 12, 2021 at 3:55 PM Luke Chen <show...@gmail.com> wrote:
> >
> > > Hi Guozhang,
> > >
> > > Thanks for your comments.
> > > I agree that in the KIP, there's a trade-off regarding the API
> > complexity.
> > > With the store impl, we can support default custom stores, but
> introduce
> > > more complexity for users, while with the enum types, users can
> configure
> > > default built-in store types easily, but it can't work for custom
> stores.
> > >
> > > For me, I'm OK to narrow down the scope and introduce the default
> > built-in
> > > enum store types first.
> > > And if there's further request, we can consider a better way to support
> > > default store impl.
> > >
> > > I'll update the KIP next week, unless there are other opinions from
> other
> > > members.
> > >
> > > Thank you.
> > > Luke
> > >
> > > On Fri, Dec 10, 2021 at 6:33 AM Guozhang Wang <wangg...@gmail.com>
> > wrote:
> > >
> > >> Thanks Luke for the updated KIP.
> > >>
> > >> One major change the new proposal has it to change the original enum
> > store
> > >> type with a new interface. Where in the enum approach our internal
> > >> implementations would be something like:
> > >>
> > >> "
> > >> Stores#keyValueBytesStoreSupplier(storeImplTypes, storeName, ...)
> > >> Stores#windowBytesStoreSupplier(storeImplTypes, storeName, ...)
> > >> Stores#sessionBytesStoreSupplier(storeImplTypes, storeName, ...)
> > >> "
> > >>
> > >> And inside the impl classes like here we would could directly do:
> > >>
> > >> "
> > >> if ((supplier = materialized.storeSupplier) == null) {
> > >> supplier =
> > >> Stores.windowBytesStoreSupplier(materialized.storeImplType())
> > >> }
> > >> "
> > >>
> > >> While I understand the benefit of having an interface such that user
> > >> customized stores could be used as the default store types as well,
> > >> there's
> > >> a trade-off I feel regarding the API complexity. Since with this
> > approach,
> > >> our API complexity granularity is in the order of "number of impl
> > types" *
> > >> "number of store types". This means that whenever we add new store
> types
> > >> in
> > >> the future, this API needs to be augmented and customized impl needs
> to
> > be
> > >> updated to support the new store types, in addition, not all custom
> impl
> > >> types may support all store types, but with this interface they are
> > forced
> > >> to either support all or explicitly throw un-supported exceptions.
> > >>
> > >> The way I see a default impl type is that, they would be safe to use
> for
> > >> any store types, and since store types are evolved by the library
> > itself,
> > >> the default impls would better be controlled by the library as well.
> > >> Custom
> > >> impl classes can choose to replace some of the stores explicitly, but
> > may
> > >> not be a best fit as the default impl classes --- if there are in the
> > >> future, one way we can consider is to make Stores class extensible
> along
> > >> with the enum so that advanced users can add more default impls,
> > assuming
> > >> such scenarios are not very common.
> > >>
> > >> So I'm personally still a bit learning towards the enum approach with
> a
> > >> narrower scope, for its simplicity as an API and also its low
> > maintenance
> > >> cost in the future. Let me know what do you think?
> > >>
> > >>
> > >> Guozhang
> > >>
> > >>
> > >> On Wed, Dec 1, 2021 at 6:48 PM Luke Chen <show...@gmail.com> wrote:
> > >>
> > >> > Hi devs,
> > >> >
> > >> > I'd like to propose a KIP to allow users to set default store
> > >> > implementation class (built-in RocksDB/InMemory, or custom one), and
> > >> > default to RocksDB state store, to keep backward compatibility.
> > >> >
> > >> > 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
> > >> >
> > >> > Any feedback and comments are welcome.
> > >> >
> > >> > Thank you.
> > >> > Luke
> > >> >
> > >>
> > >>
> > >> --
> > >> -- Guozhang
> > >>
> > >
> >
>
>
> --
> -- Guozhang
>
