Skip to content

KAFKA-8410: Migrating stateless operators to new Processor API#10381

Merged
vvcephei merged 16 commits intoapache:trunkfrom
jeqo:new-processor-kstream-1
Apr 9, 2021
Merged

KAFKA-8410: Migrating stateless operators to new Processor API#10381
vvcephei merged 16 commits intoapache:trunkfrom
jeqo:new-processor-kstream-1

Conversation

@jeqo
Copy link
Copy Markdown
Contributor

@jeqo jeqo commented Mar 23, 2021

Migrating KStream stateless operators to new Processor API, first. Following PRs will complete migration of KStream stateful operators and KTable.

Testing strategy: Keep the current tests green.

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 Mar 27, 2021
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks so much for picking this up, @jeqo !

It looks great to me, although I'd like to tighten the screws slightly on AbstractProcessor and the generic parameters when we don't need to forward anything.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just FYI, @jeqo , during my POC, I tried to fix this right away, and it dragged me into migrating the entire DSL at once. I think we should do what you are doing instead: just migrate the individual processors first, and then come back and drop the unsafe casts in a later PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One minor thing that bugged me about the old PAPI was the AbstractProcessor class. It was widely used in places where it provided no value. I'm worried that we will just perpetuate here.

Since the new API has default implementations for init and close, the only value this new abstract class provides is when the subclass needs the context. In that case, we save them from the boilerplace of saving off the context in a field.

Therefore, I'd like to call this class ContextualProcessor instead of AbstractProcessor. This should make it a little harder to depend on this class unnecessarily.

Suggested change
public abstract class AbstractProcessor<KIn, VIn, KOut, VOut> implements Processor<KIn, VIn, KOut, VOut> {
public abstract class ContextualProcessor<KIn, VIn, KOut, VOut> implements Processor<KIn, VIn, KOut, VOut> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Really like this suggestion. Thanks @vvcephei!

Comment thread streams/src/main/java/org/apache/kafka/streams/kstream/internals/KStreamPeek.java Outdated
@jeqo jeqo requested a review from vvcephei March 31, 2021 10:48
@jeqo jeqo changed the title KAFKA-12533: Migrating KStream Stateless operators to new Processor API KAFKA-8410: Migrating KStream Stateless operators to new Processor API Mar 31, 2021
@jeqo jeqo force-pushed the new-processor-kstream-1 branch from b9487a7 to e174691 Compare March 31, 2021 13:11
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks, @jeqo !

@vvcephei
Copy link
Copy Markdown
Contributor

Looks like the builds failed due to a checkstyle problem.

FYI, you can run ./gradlew :streams:testAll to run all the tests in all the Streams modules, as well as checkstyle and findbugs.

@jeqo jeqo marked this pull request as ready for review April 1, 2021 20:26
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you, @jeqo .

@vvcephei vvcephei merged commit c9cab2b into apache:trunk Apr 9, 2021
@jeqo jeqo deleted the new-processor-kstream-1 branch April 9, 2021 21:48
@jeqo jeqo changed the title KAFKA-8410: Migrating KStream Stateless operators to new Processor API KAFKA-8410: Migrating stateless operators to new Processor API Apr 12, 2021
@vvcephei vvcephei mentioned this pull request Jun 11, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants