Hi everybody, this KIP is looking good to me. Just note that the release version is set to 3.8 and should probably be updated.
Are we ready to open a vote on this one? I do not have any additional comments. Cheers, Lucas On Mon, Jun 24, 2024 at 12:55 AM Shashwat Pandey <shashwat.pandey....@gmail.com> wrote: > > Totally agree on the constructors to pass in key/val/ts/headers. > > I was thinking about adding the method for Record as a helper method if > there was a scenario to test both a processor and fixedkeyprocessor at the > same time. Also, it keeps the same signature as the > InternalFixedKeyRecordFactory as well. > > > Regards, > Shashwat Pandey > > > On Sat, Jun 22, 2024 at 8:25 PM Matthias J. Sax <mj...@apache.org> wrote: > > > Thanks for the update. > > > > About the wiki account. Creating the account was done by Infra, but > > setting permissions is on us. Fixed. > > > > > > About the KIP: > > > > Should we pass-in key/value/ts/headers and mimic the constructors for > > `TestRecord` (ie, have similar createXxx(...) overloads as factory > > methods as the constructor overloads) instead of `Record` and > > `TestRecord`? Or is there some benefit I am missing to pass in > > `Record/TestRecord` ? > > > > > > -Matthias > > > > > > On 6/22/24 9:32 AM, Shashwat Pandey wrote: > > > Hi Matthias, > > > > > > That makes sense to me! I updated the code, definitely want to get your > > > perspective on whether or not we want to support the > > > `createFixedKeyRecord(Record)` method, since we already have the > > > `TestRecord` defined in the utils, it might be cleaner to just support > > the > > > `createFixedKeyRecord(TestRecord)` method. > > > > > > For reference - > > > > > https://github.com/s7pandey/kafka/commit/8ac92509d455d8175381a9b4c83900218941bf05#diff-2a3e6e23894a888e8c2fa486e2330f42b8fb28fe2216ba182e27d3d14958457b > > > > > > Also, looks like I do not have access to update the KIP, my confluence > > > account is active now (s7pandey) but I think I need some permissions on > > the > > > actual KIP page. > > > > > > Shashwat > > > > > > On Wed, Jun 12, 2024 at 8:32 PM Matthias J. Sax <mj...@apache.org> > > wrote: > > > > > >> I believe the class name was picked on purpose, to make clear that it > > >> should not be used -- the problem is, that the class is in a public > > >> package and is by itself public (that's unfortunately require, given how > > >> Java works). > > >> > > >> Of course, it's also in the JavaDocs that the class is internal and > > >> should not be used, but not everyone reads the JavaDocs necessarily, so > > >> making it part of the name makes it much more explicit, what I believe > > >> is a good thing. > > >> > > >> I would consider it a fix/improvement, if we could exclude > > >> `InternalFixedKeyRecordFactory` from JavaDoc generation during the > > >> release build -- but I don't think we need a KIP for this, as I would > > >> rather consider it a bug-fix to exclude an internal class in the > > >> JavaDocs build step. > > >> > > >> > > >> -Matthias > > >> > > >> On 6/12/24 4:47 PM, Shashwat Pandey wrote: > > >>> 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 > > >>>>>>>>>>>>>> > > >>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>> > > >>>>>>> > > >>>>>> > > >>>>>> > > >>>> > > >>> > > >> > > > > > > > >
