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-12829: Remove deprecated Topology#addProcessor of old Processor API #17190

Closed
wants to merge 1 commit into from
Closed

KAFKA-12829: Remove deprecated Topology#addProcessor of old Processor API #17190

wants to merge 1 commit into from

Conversation

pegasas
Copy link
Contributor

@pegasas pegasas commented Sep 13, 2024 •
edited
Loading

More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.

Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@mjsax mjsax added the streams label Sep 28, 2024
@mjsax mjsax changed the title KAFKA-12829: Remove deprecated Topology#addProcessor of old Processor… KAFKA-12829: Remove deprecated Topology#addProcessor of old Processor API Sep 28, 2024
mjsax
mjsax reviewed Sep 28, 2024
View reviewed changes
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Overall LGTM. I think we can remove more unused code though.

streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java
topology
.addSource("source1", STRING_DESERIALIZER, STRING_DESERIALIZER, INPUT_TOPIC_1)
.addSource("source2", STRING_DESERIALIZER, STRING_DESERIALIZER, INPUT_TOPIC_2)
.addProcessor("processor1", defineWithStoresOldAPI(() -> new OldAPIStatefulProcessor(storeName), Collections.singleton(storeBuilder)), "source1")
Copy link
Member

Choose a reason for hiding this comment

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

OldAPIStatefulProcessor should be unused now and can be removed, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, remove these deprecated API.

streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java
private Topology createStatefulTopology(final String storeName) {
return topology
.addSource("source", STRING_DESERIALIZER, STRING_DESERIALIZER, INPUT_TOPIC_1)
.addProcessor("processor", define(new OldAPIStatefulProcessor(storeName)), "source")
Copy link
Member

Choose a reason for hiding this comment

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

define() should be unused now and can be removed, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java
final StoreBuilder<KeyValueStore<String, String>> storeBuilder = Stores.keyValueStoreBuilder(Stores.inMemoryKeyValueStore(storeName), Serdes.String(), Serdes.String());
return topology
.addSource("source", STRING_DESERIALIZER, STRING_DESERIALIZER, INPUT_TOPIC_1)
.addProcessor("processor", defineWithStoresOldAPI(() -> new OldAPIStatefulProcessor(storeName), Collections.singleton(storeBuilder)), "source")
Copy link
Member

Choose a reason for hiding this comment

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

defineWithStoresOldAPI() should be unused now and can be removed, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@pegasas
Copy link
Contributor Author

pegasas commented Oct 6, 2024

image
passing CI on local.

mjsax
mjsax reviewed Oct 16, 2024
View reviewed changes
streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java
@@ -312,79 +312,6 @@ public void testDrivingSimpleTopologyWithDroppingPartitioner() {
assertTrue(outputTopic1.isEmpty());
}

@Test
public void testDrivingStatefulTopology() {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this test? Seems we should keep it. It's not marked as @Deprecated and does not say "old API" in it's name either, nor is their a comment that it would test old API.

Copy link
Member

Choose a reason for hiding this comment

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

@pegasas -- i don't see any reply on this comment (same for the other 3 below)

streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java
}

@Test
public void testDrivingConnectedStateStoreTopology() {
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
Member

Choose a reason for hiding this comment

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

@pegasas -- i don't see any reply on this comment (2ed one)

streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java Outdated
@@ -1776,27 +1229,6 @@ public void process(final Record<String, String> record) {
}
}

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

Seems we should not remove this (it's not @Deprecated) and comment says Need to be migrated, but rather rewrite to for the new processor API? Or is it unused now?

Copy link
Member

Choose a reason for hiding this comment

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

@pegasas -- i don't see any reply on this comment (3re one)

streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java Outdated
return () -> processor;
}

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

Choose a reason for hiding this comment

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

@pegasas -- i don't see any reply on this comment (last one)

@mjsax
Copy link
Member

mjsax commented Oct 24, 2024

@pegasas -- Any updates on this PR?

@mjsax
Copy link
Member

mjsax commented Oct 30, 2024

@pegasas We are slowly approaching AK 4.0 release deadline. Would be great to push this over the finish line soon.

@pegasas
Copy link
Contributor Author

pegasas commented Oct 30, 2024

@pegasas We are slowly approaching AK 4.0 release deadline. Would be great to push this over the finish line soon.

Sorry for the delay due to busy last month.
Has replied your comments.

@mjsax
Copy link
Member

mjsax commented Nov 2, 2024

@pegasas -- I don't see any replies to the latest comment, nor an update to the PR?

@pegasas
Copy link
Contributor Author

pegasas commented Nov 2, 2024

@pegasas -- I don't see any replies to the latest comment, nor an update to the PR?

yes, I've replied to previous comments.
It seems these changes are expected.

@mjsax mjsax mentioned this pull request Nov 6, 2024
Merged
@mjsax
Copy link
Member

mjsax commented Nov 15, 2024

@pegasas -- I am still somewhat confused. Did you see my latest in-line tagging?

These are still open questions that I think we need to resolve before we can merge this PR. And AK 4.0 release deadline is approaching, so I would like to finish this :)

@pegasas
Copy link
Contributor Author

pegasas commented Nov 18, 2024

@pegasas -- I am still somewhat confused. Did you see my latest in-line tagging?

These are still open questions that I think we need to resolve before we can merge this PR. And AK 4.0 release deadline is approaching, so I would like to finish this :)

Hi, @mjsax ,

as line318 shows,

driver = new TopologyTestDriver(createStatefulTopology(storeName), props);

which createStatefulTopology would be deprecated.

https://github.com/apache/kafka/blob/trunk/streams/src/test/java/org/apache/kafka/streams/processor/internals/ProcessorTopologyTest.java#L1571-L1578

It seems my comments are disappeared several times. it is weird.

@mjsax
Copy link
Member

mjsax commented Nov 18, 2024

I believe the annotation and comment is wrong. While it does test the old API, there is no corresponding test for the new API (or did I miss it?), and thus, it should have be @SuppressWarning("deprecation") (instead of @Deprecated) and the comment should say // Old PAPI. Needs to be migrated. similar to the other cases for which I did leave a comment.

We should not remove these 4 tests, but rather rewrite all 4 to use the new PAPI, to avoid reducing test coverage. For the other test you remove in this PR, eg, testDrivingConnectedStateStoreInDifferentProcessorsTopologyWithOldAPI, there is already equivalent tests for the new PAPI (testDrivingConnectedStateStoreInDifferentProcessorsTopology) so it's ok to remove the "duplicate" test using the old API.

Does this make sense?

@mjsax
Copy link
Member

mjsax commented Dec 2, 2024

@pegasas -- did you see my last reply?

@pegasas
Copy link
Contributor Author

pegasas commented Dec 5, 2024

@pegasas -- did you see my last reply?

image
sorry for being busy at work these days.
It seems kafka uses JDK17 as default, spent some time on working on build.

Correct and fixed.

@pegasas
Copy link
Contributor Author

pegasas commented Dec 5, 2024

I believe the annotation and comment is wrong. While it does test the old API, there is no corresponding test for the new API (or did I miss it?), and thus, it should have be @SuppressWarning("deprecation") (instead of @Deprecated) and the comment should say // Old PAPI. Needs to be migrated. similar to the other cases for which I did leave a comment.

We should not remove these 4 tests, but rather rewrite all 4 to use the new PAPI, to avoid reducing test coverage. For the other test you remove in this PR, eg, testDrivingConnectedStateStoreInDifferentProcessorsTopologyWithOldAPI, there is already equivalent tests for the new PAPI (testDrivingConnectedStateStoreInDifferentProcessorsTopology) so it's ok to remove the "duplicate" test using the old API.

Does this make sense?

although the method you mentioned is SuppressWarning, but actually related class has been deprecated. maybe I think we should remove this as well.
image
image

@mjsax
Copy link
Member

mjsax commented Dec 6, 2024

It seems kafka uses JDK17 as default, spent some time on working on build.

Yes, these changes got merged very recently. Support for older Java version gets dropped with 4.0.0 release, and thus JDK17 is now required to compile trunk.

Good find about AbstractProcessor -- I did miss this one -- yes, we should remove it, including the whole MockProcessorContextTest which seems to only test deprecated API that we remove.

@mjsax
Copy link
Member

mjsax commented Dec 7, 2024

Your PR update does not seem to be right/complete? I am wondering if we have some communication gap and going somewhat in cycles? -- Would you mind if we close this PR and I work on a PR on my side directly? It's two months since this PR was opened and it seems we do not really make any progress on it.

@pegasas
Copy link
Contributor Author

pegasas commented Dec 7, 2024

Your PR update does not seem to be right/complete? I am wondering if we have some communication gap and going somewhat in cycles? -- Would you mind if we close this PR and I work on a PR on my side directly? It's two months since this PR was opened and it seems we do not really make any progress on it.

sorry.
maybe this needs to overwrite some test class which i may not have context about.
i closed this one.

@pegasas pegasas closed this Dec 7, 2024
@mjsax
Copy link
Member

mjsax commented Dec 9, 2024

Thanks for your understanding. And thanks for contributing!

@mjsax
Copy link
Member

mjsax commented Dec 12, 2024

New PR for this work item: #18154

In case you are interested. Maybe my comments and change requests make more sense now?

@pegasas
Copy link
Contributor Author

pegasas commented Dec 13, 2024

New PR for this work item: #18154

In case you are interested. Maybe my comments and change requests make more sense now?

Thanks mjsax!
I've got how these can be worked on that PR.

@pegasas pegasas deleted the kafka-12822-3 branch December 13, 2024 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjsax mjsax mjsax left review comments

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
@pegasas @mjsax