Thanks for the feedback all -- I have updated the KIP with everything mentioned so far, and added two static convenience methods for users to use to convert a ProcessorSupplier into a WrappedProcessorSupplier
Let me know if you have any more questions or suggestions. By the way, since it was mentioned briefly in this thread already, I did write up a plan for how we can clean up the TopologyConfig and topology-specific configs situation. If you're interested, KAFKA-18053 <https://issues.apache.org/jira/browse/KAFKA-18053> is an umbrella ticket with the problem description and its two subtasks describe the proposed improvements. One covers what we can do without the KIP to help things ASAP, the other covers the API cleanup that will require a KIP On Wed, Nov 20, 2024 at 5:07 PM Sophie Blee-Goldman <sop...@responsive.dev> wrote: > Bruno -- I updated the javadocs to remove the wrapping requirement. Thank > you for pointing this out > > Almog -- yeah that makes a lot of sense to me, especially being able to > add methods for the wrapper without having to add anything to the > ProcessorSupplier classes. I'm updated the KIP with this suggestion. > > On Wed, Nov 20, 2024 at 4:50 PM Almog Gavra <almog.ga...@gmail.com> wrote: > >> Hi Sophie, >> >> I was thinking it might also be good for us to return from the wrapped >> methods new marker interfaces like WrappedProcesserSupplier and >> WrappedFixedKeyProcesserSupplier. These will implement ProcesserSupplier >> and FixedKeyProcesserSupplier respectively with no additional methods >> (yet), but allows us to check whether or not something has already been >> wrapped in case it gets wrapped twice. >> >> Cheers, >> Almog >> >> On Wed, Nov 20, 2024 at 1:13 PM Bruno Cadonna <cado...@apache.org> wrote: >> >> > Hi Sophie, >> > >> > Thanks for the details! >> > >> > Yeah, I think replacing the contract in the javadocs with a warning is >> > better. >> > >> > Best, >> > Bruno >> > >> > On 20.11.24 10:05, Sophie Blee-Goldman wrote: >> > >> >> > >> Have you considered to add an interface that instead of wrapping a >> > >> processor adds methods for pre-processing and post-processing to the >> > >> processor? >> > > >> > > >> > > The problem is that we actually want to be able to access all parts of >> > the >> > > ProcessorSupplier. This includes things like inspecting the >> StoreBuilders >> > > returned by #stores (or even replacing them, eg for >> testing/debugging), >> > as >> > > well as being able to intercept the processor's #init call and get a >> > handle >> > > on the processor context. >> > > >> > > I'm open to suggestions on how to enforce a true "wrapping" of the >> > provided >> > > processor, although as you noted, it could actually be useful to >> replace >> > it >> > > altogether rather than wrapping it, for testing/debugging scenarios. >> So >> > I'd >> > > prefer to leave it flexible and not enforce this -- I can update the >> > > javadocs to remove the contract and instead just try to emphasize that >> > > modifying the processor instead of simply wrapping it can lead to >> > > unexpected results and should be done with care. >> > > >> > > This is reminiscent of the discussion we had recently around >> KIP-1088. I >> > > think the general idea holds here as well -- we should not try to >> > enforce a >> > > pure wrapper (either in code or in docs/contract) and just try to >> > > communicate the potential dangers of not doing so. >> > > >> > > On Tue, Nov 19, 2024 at 10:58 PM Bruno Cadonna <cado...@apache.org> >> > wrote: >> > > >> > >> Hi Sophie, >> > >> >> > >> Thanks for the KIP! >> > >> >> > >> I am always a bit skeptical of contracts that are defined in the Java >> > >> docs, but not enforced programmatically. I am talking about the >> contract >> > >> that requires the wrapper to return the same processor supplier >> > instance. >> > >> >> > >> Have you considered to add an interface that instead of wrapping a >> > >> processor adds methods for pre-processing and post-processing to the >> > >> processor? >> > >> >> > >> Or do you want to reconsider the contract? >> > >> >> > >> Lucas and discussed it yesterday and Lucas noted that returning a >> no-op >> > >> processor instead of the original processor might be useful for >> > debugging. >> > >> >> > >> Best, >> > >> Bruno >> > >> >> > >> >> > >> On 19.11.24 13:03, Lucas Brutschy wrote: >> > >>> Hey, >> > >>> >> > >>> I'm a bit confused since the motivation promises a cleanup of >> > >>> topologyconfig/streams config, but this doesn't seem to be part of >> this >> > >>> kip. But on the whole, this change looks fine for me. >> > >>> >> > >>> Thanks for the KIP! >> > >>> >> > >>> Cheers, >> > >>> Lucas, >> > >>> >> > >>> Sophie Blee-Goldman <sop...@responsive.dev> schrieb am Di., 19. >> Nov. >> > >> 2024, >> > >>> 00:48: >> > >>> >> > >>>> One more small change -- originally I proposed to only add this >> config >> > >> to >> > >>>> the TopologyConfig, and not to the StreamsConfig. However while >> > >>>> implementing a POC I noticed that TopologyConfig does not have a >> > >>>> constructor that accepts a plain properties or config map, and only >> > one >> > >>>> that takes in a StreamsConfig. Rather than adding a new constructor >> > for >> > >>>> TopologyConfig I think it makes sense to just add this new config >> to >> > >> both >> > >>>> StreamsConfig and TopologyConfig, as most people will generally >> want >> > to >> > >>>> create their TopologyConfig from a shared global StreamsConfig, >> rather >> > >> than >> > >>>> instantiating separate sets of configs. >> > >>>> >> > >>>> I have made this change in the KIP so please take a look and let me >> > >> know if >> > >>>> you have any concerns. Happy to discuss alternatives >> > >>>> >> > >>>> On Mon, Nov 18, 2024 at 3:27 PM Sophie Blee-Goldman < >> > >> sop...@responsive.dev >> > >>>>> >> > >>>> wrote: >> > >>>> >> > >>>>> Thanks Almog! That makes sense to me, I've updated the KIP so that >> > the >> > >>>>> ProcessorWrapper will extend Configurable but gave it a default >> no-op >> > >>>>> implementation so that it's optional. >> > >>>>> >> > >>>>> On Mon, Nov 18, 2024 at 1:57 PM Almog Gavra < >> almog.ga...@gmail.com> >> > >>>> wrote: >> > >>>>> >> > >>>>>> Thanks Sophie! This KIP will certainly make it easier to >> implement >> > any >> > >>>>>> kind >> > >>>>>> of custom functionality across all processors in the DSL, I can >> > >> imagine >> > >>>>>> quite a few use cases for this. >> > >>>>>> >> > >>>>>> One suggestion, we should consider including a configure() method >> > that >> > >>>>>> takes in Map<String, ?> configs, so that it can be configured >> based >> > on >> > >>>>>> things like application.id (e.g. for emitting custom metrics per >> > >>>>>> processor). >> > >>>>>> >> > >>>>>> - Almog >> > >>>>>> >> > >>>>>> On Fri, Nov 15, 2024 at 10:16 PM Sophie Blee-Goldman < >> > >>>>>> sop...@responsive.dev> >> > >>>>>> wrote: >> > >>>>>> >> > >>>>>>> Hey all, >> > >>>>>>> >> > >>>>>>> We have a short KIP we'd like to propose which will allow >> injecting >> > >>>>>> custom >> > >>>>>>> code modules around the processors of Kafka Streams >> applications, >> > >>>>>> including >> > >>>>>>> DSL-built topologies. >> > >>>>>>> >> > >>>>>>> Please let us know if you have any thoughts or concerns >> > >>>>>>> >> > >>>>>>> <goog_579523372> >> > >>>>>>> >> > >>>>>>> >> > >>>>>> >> > >>>> >> > >> >> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1112%3A+allow+custom+processor+wrapping >> > >>>>>>> >> > >>>>>>> Cheers, >> > >>>>>>> Sophie >> > >>>>>>> >> > >>>>>> >> > >>>>> >> > >>>> >> > >>> >> > >> >> > >> >> > > >> > >> > >> >
