kafka icon indicating copy to clipboard operation
kafka copied to clipboard

KAFKA-16339: [1/4 KStream#transform] Remove Deprecated "transformer" methods and classes

Open fonsdant opened this issue 1 year ago • 11 comments

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!

fonsdant avatar Sep 15 '24 00:09 fonsdant

I have missed removing the references to transform on Javadocs too. I will push another commit for it.

fonsdant avatar Sep 16 '24 00:09 fonsdant

I have noticed that KStreamTransformIntegrationTest.TestTransformer could be removed as it has become unused.

fonsdant avatar Sep 17 '24 22:09 fonsdant

For now, I have removed the build with deduplicaiton topology test until we decide how to replace the transform method inside it.

fonsdant avatar Sep 17 '24 23:09 fonsdant

I have replaced tranform with process as suggested by docs. I have implemented Processor based on StandbyTaskEOSMultiRebalanceIntegrationTest.buildWithUniqueIdAssignmentTopology, but shouldWipeOutStandbyStateDirectoryIfCheckpointIsMissing has failed.

fonsdant avatar Sep 20 '24 00:09 fonsdant

I have managed to fix the test! :D

fonsdant avatar Sep 20 '24 01:09 fonsdant

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?

fonsdant avatar Sep 23 '24 16:09 fonsdant

I will push some commits, one for each file from which I have removed the transform link, replacing it with process.

fonsdant avatar Oct 08 '24 11:10 fonsdant

I have rebased my branch on trunk and this caused GitHub to close the PR. I am implementing the last review points again.

fonsdant avatar Oct 14 '24 21:10 fonsdant

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 avatar Oct 15 '24 12:10 fonsdant

@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()

mjsax avatar Oct 18 '24 22:10 mjsax

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?

fonsdant avatar Oct 19 '24 19:10 fonsdant

We can keep this PR as-is -- you can also to the refactoring as part of this PR if you want.

mjsax avatar Oct 22 '24 00:10 mjsax

@fonsdant Any update on this PR? -- We are approaching AK 4.0 release deadlines, and there is 3 more PRs for this ticket...

mjsax avatar Oct 30 '24 06:10 mjsax

@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.

fonsdant avatar Oct 30 '24 10:10 fonsdant

I will push the work I done this passed week.

fonsdant avatar Oct 30 '24 23:10 fonsdant

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.

mjsax avatar Oct 31 '24 00:10 mjsax

I have managed to refactor the tests! 😄

Now, I will just update the Javadocs and Scaladocs again

fonsdant avatar Oct 31 '24 00:10 fonsdant

@fonsdant There is merge conflicts -- can you rebase to latest trunk?

mjsax avatar Nov 02 '24 00:11 mjsax

@mjsax, rebased!

fonsdant avatar Nov 04 '24 11:11 fonsdant

@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.

fonsdant avatar Nov 05 '24 01:11 fonsdant

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?

fonsdant avatar Nov 05 '24 10:11 fonsdant

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

mjsax avatar Nov 06 '24 06:11 mjsax

Uhuu! Here we go! I need to rebase the second one on this first now. Thanks, @mjsax!

fonsdant avatar Nov 06 '24 09:11 fonsdant