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