Hi Walker,
I have follow-up comments.
1.
I think, we should add an overload to the StreamsBuilder class that
allows to name the processor with Named. That makes that processor
consistent with all other processors in the DSL regarding naming.
2.
I do not understand what you mean with "maximum flexibility". The
built-in processor needs to assume a given state store interface. That
means, users have to provide a state store that offers that interface.
If they do not they will get a runtime exception. If we require a store
builder for a given interface, we can catch the mistake at compile time.
Let me know whether I misunderstood something.
Best,
Bruno
On 3/25/24 7:05 PM, Walker Carlson wrote:
Hey Bruno,
1) I'm actually not sure why that is in there. It certainly doesn't match
the convention. Best to remove it and match the other methods.
2) Yeah, I thought about it but I'm not convinced it is a necessary
restriction. It might be useful for the already defined processors but then
they might as well use the `globalTable` method. I think the add state
store option should go for maximum flexibility.
Best,
Walker
On Fri, Mar 22, 2024 at 10:01 AM Bruno Cadonna <cado...@apache.org> wrote:
Hi Walker,
A couple of follow-up questions.
1.
Why do you propose to explicitly pass a parameter "storeName" in
StreamsBuilder#addGlobalStore?
The StoreBuilder should already provide a name for the store, if I
understand the code correctly.
I would avoid using the same name for the source node and the state
store, because it limits the flexibility in naming. Why do you not use
Named for the name of the source node?
2.
Did you consider Matthias' proposal to restrict the type of the store
builder to `StoreBuilder<TimestampedKeyValueStore>` (or even
`StoreBuilder<? extends TimestampedKeyValueStore>`) for the case where
the processor is built-in?
Best,
Bruno
On 3/13/24 11:05 PM, Walker Carlson wrote:
Thanks for the feedback Bruno, Matthias, and Lucas!
There is a decent amount but I'm going to try and just hit the major
points
as I would like to keep this change simple.
I've made corrections for the mistakes pointed out. Thanks for the
suggestions everyone.
The main sticking point seems to be with the method of signalling the
restore behavior. It seems we can all agree with how the API should look
with the default option we are adding. I think keeping the option to load
directly from the topic into the store is a good idea. It is much more
performant and could make a simple metric collector processor much
simpler.
I think something that Matthais said about creating a special class of
processors for the global stores helps me think about the issue. I tend
to
fall into the category that we should keep global stores open to the
possibility of having child nodes in the future. I don't really see the
downside of having that as an option. It might not be best for a lot of
cases, but something simple could be very useful to put in the PAPI.
I like the idea of having a `GlobalStoreParameters` but only if we decide
to make the processor need to extend an interface like
'GobalStoreProcessor`. If not that seems excessive.
As of right now I don't see a better option than having a boolean flag
for
the reprocessOnRestore option. I expanded the description in the docs so
I
hope that helps.
I am more than willing to take other ideas on it.
thanks,
Walker
