Hi Matthias, I think that strategy definitely works, abstracting away changes to FixedKeyRecord from users, I can put that new factory class and update the KIP.
This might be a discussion for another KIP, but would it also make sense to rename the InternalFixedKeyRecordFactory to just FixedKeyRecordFactory also make sense? Regards, Shashwat Pandey On Mon, Jun 10, 2024 at 5:07 PM Matthias J. Sax <mj...@apache.org> wrote: > Shaswhat, > > any updates on this KIP? -- I still think that recommending to use > `InternalFixedKeyRecordFactory` is not the best way to write test code. > Changing `FixedKeyRecord` constructor (as I mentioned in my last email) > might not be a good solution either. > > Maybe a cleaner way would be (so sidestep this problem), to add a new > public "factory class" into the test package to generate > FixedKeyRecords, and this factory could internally use > `InternalFixedKeyRecordFactory`? It looks cleaner to me from an API POV, > and if we change anything how `FixedKeyRecord` can be created, it would > become a non-user-facing / internal change to the "factory" we provide. > > > -Matthias > > On 5/22/24 12:02 AM, Matthias J. Sax wrote: > > I was not aware of `InternalFixedKeyRecordFactory`. As the name > > indicates, it's considered an internal class, so not sure if we should > > recommend to use it in test... > > > > I understand why this class is required, and why it was put into a > > public package; the way Java works, enforces this. Not sure if we could > > find a better solution. > > > > Might be good to hear from others. > > > > > > -Matthias > > > > On 5/21/24 3:57 PM, Shashwat Pandey wrote: > >> Looking at the ticket and the sample code, I think it would be > >> possible to > >> continue using `InternalFixedKeyRecordFactory` as the avenue to create > >> `FixedKeyRecord`s in tests. As long as there was a > >> MockFixedKeyProcessorContext, I think we would be able to test > >> FixedKeyProcessors without a Topology. > >> > >> I created a sample repo with the `MockFixedKeyProcessorContext` here is > >> what I think the tests would look like: > >> > https://github.com/s7pandey/kafka-processor-tests/blob/main/src/test/java/com/example/demo/MyFixedKeyProcessorTest.java > >> > >> > >> > >> On Mon, May 20, 2024 at 9:05 PM Matthias J. Sax <mj...@apache.org> > wrote: > >> > >>> Had a discussion on https://issues.apache.org/jira/browse/KAFKA-15242 > >>> and it was pointed out, that we also need to do something about > >>> `FixedKeyRecord`. It does not have a public constructor (what is > >>> correct; it should not have one). However, this makes testing > >>> `FixedKeyProcessor` impossible w/o extending `FixedKeyRecord` manually > >>> what does not seem to be right (too clumsy). > >>> > >>> It seems, we either need some helper builder method (but not clear to > me > >>> where to add it in an elegant way) which would provide us with a > >>> `FixedKeyRecord`, or add some sub-class to the test-utils module which > >>> would extend `FixedKeyRecord`? -- Or maybe an even better solution? I > >>> could not think of something else so far. > >>> > >>> > >>> Thoughts? > >>> > >>> > >>> On 5/3/24 9:46 AM, Matthias J. Sax wrote: > >>>> Please also update the KIP. > >>>> > >>>> To get a wiki account created, please request it via a commet on this > >>>> ticket: https://issues.apache.org/jira/browse/INFRA-25451 > >>>> > >>>> After you have the account, please share your wiki id, and we can give > >>>> you write permission on the wiki. > >>>> > >>>> > >>>> > >>>> -Matthias > >>>> > >>>> On 5/3/24 6:30 AM, Shashwat Pandey wrote: > >>>>> 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 > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>> > >>>>> > >>> > >> > >> >
