kafka
kafka copied to clipboard
KAFKA-16339: [1/4 KStream#transform] Remove Deprecated "transformer" methods and classes
Hi, @mjsax! I have removed the transform methods from the main and test classes. However, I have asked myself whether the approach I have used for StandbyTaskEOSIntegrationTest and StreamThreadTest is correct. Should I replace transform with process as suggested in transform Javadoc or remove these tests? Thanks!
I have missed removing the references to transform on Javadocs too. I will push another commit for it.
I have noticed that KStreamTransformIntegrationTest.TestTransformer could be removed as it has become unused.
For now, I have removed the build with deduplicaiton topology test until we decide how to replace the transform method inside it.
I have replaced tranform with process as suggested by docs. I have implemented Processor based on StandbyTaskEOSMultiRebalanceIntegrationTest.buildWithUniqueIdAssignmentTopology, but shouldWipeOutStandbyStateDirectoryIfCheckpointIsMissing has failed.
I have managed to fix the test! :D
I have removed the references to transform from docs. I am wondering if it would be better to replace transform with process, instead of removing it. I have started to think about it while removing the flatTransformValues (the third method to remove). @mjsax, could you help me with this decision?
I will push some commits, one for each file from which I have removed the transform link, replacing it with process.
I have rebased my branch on trunk and this caused GitHub to close the PR. I am implementing the last review points again.
This time, I have focused on removing the methods first and doing refactorings. After this, I will proceed with Javadoc updates.
I have made the removing part. Now, I will do the refactoring part.
@fonsdant -- seems there is some compilation error:
StandbyTaskEOSIntegrationTest.java:332: error: cannot find symbol
.transform(
> Task :streams:compileTestJava
^
symbol: method transform(()->new or[...]{ } },String)
location: interface KStream<Integer,Integer>
Seem we need to rewrite this test to use the new process() instead of transform()
Thanks, @mjsax, still need to refactor it. Meanwhile, do you prefer that I convert this PR to draft? Or should I keep it as it is?
We can keep this PR as-is -- you can also to the refactoring as part of this PR if you want.
@fonsdant Any update on this PR? -- We are approaching AK 4.0 release deadlines, and there is 3 more PRs for this ticket...
@mjsax, sorry for the delay, I am still refactoring the tests, replacing transform with process.
Given the coming deadline, I agree it would be adequate someone more experienced to resume it.
I will push the work I done this passed week.
Given the coming deadline, I agree it would be adequate someone more experienced to resume it.
As long as you have enough time to keep working, I think we are a good. Of course, if you are blocked, let us know.
I have managed to refactor the tests! 😄
Now, I will just update the Javadocs and Scaladocs again
@fonsdant There is merge conflicts -- can you rebase to latest trunk?
@mjsax, rebased!
@mjsax, I have removed the adding lines in buildTopologyWithDeduplication in StandbyTaskEOSIntegrationTest to match the previous implementation with transform (but still with process). Also, removed the comments of the KStreamTest scala test and rewritten it. Finally, removed var usage.
Thanks, @mjsax! I have simplified the test and replaced the processorSupplier with a lambda. Should I replace it for for other methods too? And how about IntelliJ reporting it as problem?
Merged to trunk. Thanks for the PR! One down. Three to go. :) Looking forward to your update of https://github.com/apache/kafka/pull/17245
Uhuu! Here we go! I need to rebase the second one on this first now. Thanks, @mjsax!