Thanks for the KIP!
I think it's a good step forward for the DSL and it makes sense to
exclude the PAPI and custom stores for now.
About the config name, it seems to be overly complicated. Guozhang's
argument about "store type" that is used to refer to kv, windowed,
session stores make sense. But having "type" and "impl" in the name
sounds clumsy to me. What about a simplified name:
default.dsl.store.impl
or maybe even better
default.dsl.store
for the config?
Btw: the KIP still contains a view case of the originally proposed
config name `default.dsl.store.type` which should be updated.
About `TopologyConfig`: we should add all public methods of this class,
including constructors.
-Matthias
On 12/22/21 4:54 AM, Luke Chen wrote:
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
