Hi Matthias,
Sorry this totally fell off my radar, what would be left to do?
Would updating the version to latest release be enough?
Regards,
Shashwat Pandey
On Wed, Jun 11, 2025 at 5:37 PM Matthias J. Sax <mj...@apache.org
<mailto:mj...@apache.org>> wrote:
Is there still interest to complete this KIP?
-Matthias
On 8/26/24 5:06 AM, Lucas Brutschy wrote:
> 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
<mailto: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 <mailto: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
<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 <mailto: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 <mailto: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
<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 <mailto:mj...@apache.org>>
>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Had a discussion on
>>>>> https://issues.apache.org/jira/browse/KAFKA-15242 <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 <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 <mailto: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 <mailto: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 <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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
