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
wrote:
>
> Totally agree
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 w
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
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
`createFixedKeyRecor
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
s
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 FixedKeyRe
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 s
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 i
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.
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
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
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
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
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
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 MockFixedKe
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,
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
17 matches
Mail list logo
