Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-16339: [3/4 KStream#transformValues] Remove Deprecated "transformer" methods and classes #17266

Merged
merged 17 commits into from
Nov 22, 2024
Merged

KAFKA-16339: [3/4 KStream#transformValues] Remove Deprecated "transformer" methods and classes #17266

merged 17 commits into from
Nov 22, 2024

Conversation

fonsdant
Copy link
Contributor

No description provided.

@mjsax
Copy link
Member

mjsax commented Nov 15, 2024

@fonsdant -- any updates?

@fonsdant
Copy link
Contributor Author

@mjsax, yes! I must be pushing some commits tomorrow. I am checking if there is need to refactor the tests.

@fonsdant
kstream: remove transform values
df6ff5a 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
kstream impl: remove transform values
713e370 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
streams builder test: remove transform values
5b614c9 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant fonsdant force-pushed the kstream-transform-values-remove-deprecated-transformer-methods-and-classes branch from 03959ad to 329d2ae Compare November 15, 2024 21:07
@fonsdant fonsdant marked this pull request as ready for review November 15, 2024 21:08
@fonsdant fonsdant changed the title [WIP] KAFKA-16339: [3/4 KStream#transformValues] Remove Deprecated "transformer" methods and classes KAFKA-16339: [3/4 KStream#transformValues] Remove Deprecated "transformer" methods and classes Nov 15, 2024
@fonsdant
Copy link
Contributor Author

I will push the docs update too in a while.

@fonsdant
kstream impl test: refactor and remove transform values
11c217c 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
kstream transform values test: remove transform values
565f4b6 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
kstream scala: remove transform values
a32ef69 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
kstream javadoc: refactor and remove transform values
4d1ee36 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
transform supplier javadoc: remove transform values
a7832c5 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
value transformer javadoc: remove transform values
b62e3a5 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
value transformer supplier javadoc: remove transform values
d6cf2b6 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
value transformer with key javadoc: remove transform values
273ae22 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
value transformer with key supplier javadoc: remove transform values
0a2304b 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
connected store provider javadoc: refactor and remove transform values
712f211 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant fonsdant force-pushed the kstream-transform-values-remove-deprecated-transformer-methods-and-classes branch from f1cb8f2 to 712f211 Compare November 16, 2024 03:28
mjsax
mjsax reviewed Nov 19, 2024
View reviewed changes
...ams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamTransformValuesTest.java
@@ -54,92 +44,6 @@ public class KStreamTransformValuesTest {
private final Properties props = StreamsTestUtils.getStreamsConfig(Serdes.Integer(), Serdes.Integer());
private InternalProcessorContext context = mock(InternalProcessorContext.class);

@SuppressWarnings("deprecation") // Old PAPI. Needs to be migrated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a corresponding test for process(FixedKeyProcessor) -- the comment says "needs to be migrated -- did this migration already happen?

mjsax
mjsax reviewed Nov 19, 2024
View reviewed changes
...ams/src/test/java/org/apache/kafka/streams/kstream/internals/KStreamTransformValuesTest.java
assertArrayEquals(expected, supplier.theCapturedProcessor().processed().toArray());
}

@SuppressWarnings("deprecation") // Old PAPI. Needs to be migrated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they both are covered by org.apache.kafka.streams.kstream.internals.KStreamImplTest#shouldProcessValues, since processValues just has FixedKeyProcessorSupplier as processor supplier.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. SG.

But I am wondering if we can remove KStreamTransformValuesTest.java completely for this case?

What also implies that we can remove KStreamTransformValues.java?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fonsdant Did you miss this comment?

Copy link
Contributor Author

@fonsdant fonsdant Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjsax, oh, sorry! Yes, I have missed it. Thanks!

Makes sense to me too! I'm going to push the removal.

Following this approach, we could remove KStreamFlatTransformTest and KStreamFlatTransform too, couldn't we?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following this approach, we could remove KStreamFlatTransformTest and KStreamFlatTransform too, couldn't we?

Maybe in PR [4/4], but not in this PR, right?

@mjsax
Copy link
Member

mjsax commented Nov 19, 2024

Made a pass. Overall LGTM. A few comments:

I will push the docs update too in a while.

Thx. PR [4/4] should remove flatTransformValues thought, right?

@fonsdant
link ktable transform values
45b4117 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
remove unused private method from kstream impl
a30dad1 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant fonsdant force-pushed the kstream-transform-values-remove-deprecated-transformer-methods-and-classes branch from 803a408 to a30dad1 Compare November 20, 2024 14:56
@fonsdant fonsdant requested a review from mjsax November 20, 2024 14:57
@fonsdant
remove kstream flat transform values
2ecb288 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@fonsdant
remove kstream flat transform values test
e02269a 
Signed-off-by: Joao Pedro Fonseca Dantas <[email protected]>
@mjsax mjsax merged commit 866f0cc into apache:trunk Nov 22, 2024
8 checks passed
@mjsax
Copy link
Member

mjsax commented Nov 22, 2024

Thanks for the PR. Merged to trunk.

chiacyu pushed a commit to chiacyu/kafka that referenced this pull request Nov 30, 2024
@fonsdant @chiacyu
KAFKA-16339: [3/4 KStream#transformValues] Remove Deprecated "transfo…
16f413f 
…rmer" methods and classes (apache#17266)

Reviewers: Matthias J. Sax <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjsax mjsax Awaiting requested review from mjsax

Assignees
No one assigned
Labels
ci-approved streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fonsdant @mjsax