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

pegasas · 2024-09-13T15:42:32Z

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

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

mjsax · 2024-09-28T18:39:43Z

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")


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

Thanks, remove these deprecated API.

mjsax · 2024-09-28T18:40:35Z

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")


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

mjsax · 2024-09-28T18:41:26Z

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")


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

… API

pegasas · 2024-10-06T14:55:29Z

passing CI on local.

mjsax · 2024-10-16T21:05:42Z

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


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.

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

mjsax · 2024-10-16T21:05:48Z

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

-    }
-
-    @Test
-    public void testDrivingConnectedStateStoreTopology() {


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

mjsax · 2024-10-16T21:07:52Z

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

@@ -1776,27 +1229,6 @@ public void process(final Record<String, String> record) {
        }
    }

-    @SuppressWarnings("deprecation") // Old PAPI. Needs to be migrated.


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?

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

mjsax · 2024-10-16T21:07:58Z

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

-        return () -> processor;
-    }
-
-    @SuppressWarnings("deprecation") // Old PAPI. Needs to be migrated.


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

mjsax · 2024-10-24T00:01:15Z

@pegasas -- Any updates on this PR?

mjsax · 2024-10-30T06:13:09Z

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

pegasas · 2024-10-30T11:22:46Z

@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 · 2024-11-02T00:40:21Z

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

pegasas · 2024-11-02T06:02:12Z

@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 · 2024-11-15T00:36:54Z

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

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

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 · 2024-11-18T02:09:32Z

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

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

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

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

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

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 · 2024-11-18T20:18:13Z

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?

pegasas force-pushed the kafka-12822-3 branch from cb98260 to 37e99d8 Compare September 13, 2024 15:42

mjsax added the streams label Sep 28, 2024

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 reviewed Sep 28, 2024

View reviewed changes

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

8466f4c

… API

pegasas force-pushed the kafka-12822-3 branch from 37e99d8 to 8466f4c Compare October 6, 2024 14:54

mjsax reviewed Oct 16, 2024

View reviewed changes

mjsax added the ci-approved label Oct 22, 2024

mjsax mentioned this pull request Nov 6, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pegasas commented Sep 13, 2024 •

edited

Loading

mjsax left a comment

mjsax Sep 28, 2024

pegasas Oct 6, 2024

mjsax Sep 28, 2024

pegasas Oct 6, 2024

mjsax Sep 28, 2024

pegasas Oct 6, 2024

pegasas commented Oct 6, 2024

mjsax Oct 16, 2024

mjsax Nov 4, 2024

mjsax Oct 16, 2024

mjsax Nov 4, 2024

mjsax Oct 16, 2024

mjsax Nov 4, 2024

mjsax Oct 16, 2024

mjsax Nov 4, 2024

mjsax commented Oct 24, 2024

mjsax commented Oct 30, 2024

pegasas commented Oct 30, 2024

mjsax commented Nov 2, 2024

pegasas commented Nov 2, 2024

mjsax commented Nov 15, 2024

pegasas commented Nov 18, 2024

mjsax commented Nov 18, 2024

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

Are you sure you want to change the base?

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

Conversation

pegasas commented Sep 13, 2024 • edited Loading

Committer Checklist (excluded from commit message)

mjsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pegasas commented Oct 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax commented Oct 24, 2024

mjsax commented Oct 30, 2024

pegasas commented Oct 30, 2024

mjsax commented Nov 2, 2024

pegasas commented Nov 2, 2024

mjsax commented Nov 15, 2024

pegasas commented Nov 18, 2024

mjsax commented Nov 18, 2024

pegasas commented Sep 13, 2024 •

edited

Loading