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