Hi Matthias, Sorry this fell out of my radar for a bit.
Revisiting the topic, I think you’re right and we accept the duplicated nesting as an appropriate solution to not affect the larger public API. I can update my PR with the change. Regards, Shashwat Pandey On Wed, May 1, 2024 at 11:00 PM Matthias J. Sax <mj...@apache.org> wrote: > Any updates on this KIP? > > On 3/28/24 4:11 AM, Matthias J. Sax wrote: > > It seems that `MockRecordMetadata` is a private class, and thus not part > > of the public API. If there are any changes required, we don't need to > > discuss on the KIP. > > > > > > For `CapturedPunctuator` and `CapturedForward` it's a little bit more > > tricky. My gut feeling is, that the classes might not need to be > > changed, but if we use them within `MockProcessorContext` and > > `MockFixedKeyProcessorContext` it might be weird to keep the current > > nesting... The problem I see is, that it's not straightforward how to > > move the classes w/o breaking compatibility, nor if we duplicate them as > > standalone classes w/o a larger "splash radius". (We would need to add > > new overloads for MockProcessorContext#scheduledPunctuators() and > > MockProcessorContext#forwarded()). > > > > Might be good to hear from others if we think it's worth this larger > > changes to get rid of the nesting, or just accept the somewhat not ideal > > nesting as it technically is not a real issue? > > > > > > -Matthias > > > > > > On 3/15/24 1:47 AM, Shashwat Pandey wrote: > >> Thanks for the feedback Matthias! > >> > >> The reason I proposed the extension of MockProcessorContext was more > >> to do > >> with the internals of the class (MockRecordMetadata, > >> CapturedPunctuator and > >> CapturedForward). > >> > >> However, I do see your point, I would then think to split > >> MockProcessorContext and MockFixedKeyProcessorContext, some of the > >> internal > >> classes should also be extracted i.e. MockRecordMetadata, > >> CapturedPunctuator and probably a new CapturedFixedKeyForward. > >> > >> Let me know what you think! > >> > >> > >> Regards, > >> Shashwat Pandey > >> > >> > >> On Mon, Mar 11, 2024 at 10:09 PM Matthias J. Sax <mj...@apache.org> > >> wrote: > >> > >>> Thanks for the KIP Shashwat. Closing this testing gap is great! It did > >>> come up a few time already... > >>> > >>> One question: why do you propose to `extend MockProcessorContext`? > >>> > >>> Given how the actual runtime context classes are setup, it seems that > >>> the regular context and fixed-key-context are distinct, and thus I > >>> believe both mock-context classes should be distinct, too? > >>> > >>> What I mean is that FixedKeyProcessorContext does not extend > >>> ProcessorContext. Both classes have a common parent ProcessINGContext > >>> (note the very similar but different names), but they are "siblings" > >>> only, so why make the mock processor a parent-child relationship? > >>> > >>> It seems better to do > >>> > >>> public class MockFixedKeyProcessorContext<KForward, VForward> > >>> implements FixedKeyProcessorContext<KForward, VForward>, > >>> RecordCollector.Supplier > >>> > >>> > >>> Of course, if there is code we can share between both mock-context we > >>> should so this, but it should not leak into the public API? > >>> > >>> > >>> -Matthias > >>> > >>> > >>> > >>> On 3/11/24 5:21 PM, Shashwat Pandey wrote: > >>>> Hi everyone, > >>>> > >>>> I would like to start the discussion on > >>>> > >>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1027%3A+Add+MockFixedKeyProcessorContext > >>>> > >>>> This adds MockFixedKeyProcessorContext to the Kafka Streams Test Utils > >>>> library. > >>>> > >>>> Regards, > >>>> Shashwat Pandey > >>>> > >>> > >> >
