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
>
