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.

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

KAFKA-17091: Add @FunctionalInterface to Streams interfaces #16544

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

raymcdermott
Copy link

Goal

Add @FunctionalInterface to Streams interfaces to enable Clojure 1.12 (and any other JVM language) to have the correct hints for these SAM interfaces.

The Clojure issue is here

Testing

Unit tests and system tests all pass locally.

Committer Checklist (excluded from commit message)

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

@bbejeck
Copy link
Contributor

bbejeck commented Jul 8, 2024

Hi @raymcdermott thanks for the PR! Since this is a change to the public interface code of Kafka Streams, this PR will require a KIP. Ping me here once you create a wiki ID (step 2 in the getting started section from the page referenced above) and I'll go in and give you edit permissions to create a KIP.

@C0urante C0urante added the kip Requires or implements a KIP label Jul 8, 2024
@mjsax
Copy link
Member

mjsax commented Jul 8, 2024

It seems ForeachAction is missing?

Also wondering about TransformerSupplier (and siblings) -- are they now effectively deprecated via https://cwiki.apache.org/confluence/display/KAFKA/KIP-820%3A+Extend+KStream+process+with+new+Processor+API (even if the KIP does not say so expliclity -- what might be a miss on our end to add to the KIP... Thoughts)

What about other PAPI interfaces like FixedKeyProcessor, api.Processor (not sure if both quality or not), Punctuator, StateRestoreCallback (maybe?), StreamPartitioner, TimestampExtractor, TopicNameExtractor, and IQ interface Query?

Not sure if there might be other I have missed.

@raymcdermott
Copy link
Author

raymcdermott commented Jul 17, 2024

It seems ForeachAction is missing?

Also wondering about TransformerSupplier (and siblings) -- are they not effectively deprecated via https://cwiki.apache.org/confluence/display/KAFKA/KIP-820%3A+Extend+KStream+process+with+new+Processor+API (even if the KIP does not say so expliclity -- what might be a miss on our end to add to the KIP... Thoughts)

What about other PAPI interfaces like FixedKeyProcessor, api.Processor (not sure if both quality or not), Punctuator, StateRestoreCallback (maybe?), StreamPartitioner, TimestampExtractor, TopicNameExtractor, and IQ interface Query?

Not sure if there might be other I have missed.

I attempted to have a maximal set but some of the tests broke so I pared it back to the minimum set.

Many of the mentioned PAPI interfaces do not qualify as @FunctionalInterface as they have > 1 method. Ref the Java API docs

I will however check those other interfaces to see if the annotation will break anything.

@raymcdermott
Copy link
Author

Also wondering about TransformerSupplier (and siblings) -- are they not effectively deprecated via https://cwiki.apache.org/confluence/display/KAFKA/KIP-820%3A+Extend+KStream+process+with+new+Processor+API (even if the KIP does not say so expliclity -- what might be a miss on our end to add to the KIP... Thoughts)

Libraries use those deprecated functions so I would like to include them.

@mjsax
Copy link
Member

mjsax commented Jul 18, 2024

I attempted to have a maximal set but some of the tests broke so I pared it back to the minimum set.

Curious to learn about this. I don't understand how adding an annotation could break a test?

Many of the mentioned PAPI interfaces do not qualify as @FunctionalInterface as they have > 1 method. Ref the Java API docs

But the JavaDocs says: Each functional interface has a single abstract method, called the functional method for that functional interface, to which the lambda expression's parameter and return types are matched or adapted. -- My interpretation is that "single abstract method" means, only one abstract method, but it does not mean exactly one abstract method (ie, there can also be non-abstract methods)? It's only not a functional interface if there is zero, or more than one abstract methods? So an interface with three method, and two having a default imply, would still qualify as functional interface? -- Or is my interpretation incorrect?

Also wondering about TransformerSupplier (and siblings) -- are they not effectively deprecated via https://cwiki.apache.org/confluence/display/KAFKA/KIP-820%3A+Extend+KStream+process+with+new+Processor+API (even if the KIP does not say so expliclity -- what might be a miss on our end to add to the KIP... Thoughts)

Libraries use those deprecated functions so I would like to include them.

We do plan to remove many of these things in 4.0 (I should have linked both ticket: https://issues.apache.org/jira/browse/KAFKA-12829 and https://issues.apache.org/jira/browse/KAFKA-16339) -- Thus, it might not be worth to cover them with the KIP -- but it might be good to drop a sentence in the KIP why we don't consider them (just for documentation purpose, and to have a "complete" KIP)

@raymcdermott
Copy link
Author

Also wondering about TransformerSupplier (and siblings) -- are they not effectively deprecated via https://cwiki.apache.org/confluence/display/KAFKA/KIP-820%3A+Extend+KStream+process+with+new+Processor+API (even if the KIP does not say so expliclity -- what might be a miss on our end to add to the KIP... Thoughts)
Libraries use those deprecated functions so I would like to include them.

We do plan to remove many of these things in 4.0 (I should have linked both ticket: https://issues.apache.org/jira/browse/KAFKA-12829 and https://issues.apache.org/jira/browse/KAFKA-16339) -- Thus, it might not be worth to cover them with the KIP -- but it might be good to drop a sentence in the KIP why we don't consider them (just for documentation purpose, and to have a "complete" KIP)

I'm a bit confused about the guidance here. Are you saying that we should exclude the currently deprecated interfaces and target this KIP against 4.0?

That's a bit worrying to me because those things have not yet been removed and there may be reasons for not removing them.

I was hoping that this KIP would be simple but trapping it in the timelines of releases adds a lot of complexity IMHO

@raymcdermott
Copy link
Author

Many of the mentioned PAPI interfaces do not qualify as @FunctionalInterface as they have > 1 method. Ref the Java API docs

But the JavaDocs says: Each functional interface has a single abstract method, called the functional method for that functional interface, to which the lambda expression's parameter and return types are matched or adapted. -- My interpretation is that "single abstract method" means, only one abstract method, but it does not mean exactly one abstract method (ie, there can also be non-abstract methods)? It's only not a functional interface if there is zero, or more than one abstract methods? So an interface with three method, and two having a default imply, would still qualify as functional interface? -- Or is my interpretation incorrect?

TIL that default methods would not count so I will check again. Thanks for the education.

@raymcdermott
Copy link
Author

I attempted to have a maximal set but some of the tests broke so I pared it back to the minimum set.

Curious to learn about this. I don't understand how adding an annotation could break a test?

Will write it up if it happens again as I add more @FunctionalInterface annotations.

@mjsax
Copy link
Member

mjsax commented Jul 18, 2024

I'm a bit confused about the guidance here. Are you saying that we should exclude the currently deprecated interfaces and target this KIP against 4.0?

That's a bit worrying to me because those things have not yet been removed and there may be reasons for not removing them.

I was hoping that this KIP would be simple but trapping it in the timelines of releases adds a lot of complexity IMHO

That's a fair statement. Just wanted to point it out. If your KIP gets approved and we really remove them in 4.0, it was just busy work. (And I am 99% certain that we will remove it in 4.0). -- Just wanted to avoid any surprises. -- The other aspect is: even if the methods are not removed in 4.0, the would not be any damage if we exclude them in your KIP IMHO. The methors are deprecated and should not be use any longer, so there is no need to improve them. Not adding the annotation to them could actually be a forcing function for users to use the new methods instead of the deprecated ones.

If you want to include it in the KIP, fine with me, too. Leave it up to you. (With a slight preference to exclude them.)

TIL that default methods would not count so I will check again. Thanks for the education.

That's my understanding -- but I could be wrong. Would be good to verify and double check this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kip Requires or implements a KIP streams
Projects
None yet
4 participants