KAFKA-2600 Align Kafka Streams' interfaces with Java 8 functional interfaces

[rhauch]

A few of Kafka Stream's interfaces and classes are not as well-aligned with Java 8's functional interfaces. By making these changes, when Kafka moves to Java 8 these classes can extend standard Java 8 functional interfaces while remaining backward compatible. This will make it easier for developers to use Kafka Streams, and may allow us to eventually remove these custom interfaces and just use the standard Java 8 interfaces.

The changes include:

1. The 'apply' method of KStream's Predicate functional interface was renamed to test to match the method name on java.util.function.BiPredicate. This will allow KStream's Predicate to extend BiPredicate when Kafka moves to Java 8, and for the KStream.filter and filterOut methods to accept BiPredicate.
2. Renamed the ProcessorDef and WindowDef interfaces to ProcessorSupplier and WindowSupplier, respectively. Also the SlidingWindowDef class was renamed to SlidingWindowSupplier, and the MockProcessorDef test class was renamed to MockProcessorSupplier. The instance() method in all were renamed to get(), so that all of these can extend/implement Java 8's java.util.function.Supplier<T> interface in the future with no other changes and while remaining backward compatible. Variable names that used some form of "def" were changed to use "supplier".

These two sets of changes were made in separate commits.

[ijuma]

I think it's a good idea to align our functional interfaces with the Java 8 ones. To be clear though, after we change our interfaces to extend the Java 8 ones, it won't change anything for the user.

It's only when we change the various classes and methods to accept the Java 8 functional interfaces that things actually change. This won't be a compatible change, however (we would have to use overloads to make it compatible, but that would cause issues for lambdas, so it's a no go).

Do you agree with this assessment or am I missing something?

[rhauch]

I think it's a good idea to align our functional interfaces with the Java 8 ones. To be clear though, after we change our interfaces to extend the Java 8 ones, it won't change anything for the user.
It's only when we change the various classes and methods to accept the Java 8 functional interfaces that things actually change.

Agreed. But making these changes now means that when Kafka moves to Java 8 we can change our interfaces to extend the Java 8 ones and change the interfaces to accept the Java 8 equivalents (what will be the supertype of our interfaces). And that should be a compatible change: any existing code that uses our interfaces will still work (after all, our interfaces will still be sub-interfaces of the type the methods will accept), and any new code could use either the Java 8 equivalents (preferred) or our interfaces. If we want, we could even deprecate our interfaces and encourage users to use the Java 8 equivalents, and in the subsequent major release remove those interfaces.

This won't be a compatible change, however (we would have to use overloads to make it compatible, but that would cause issues for lambdas, so it's a no go).

Actually, the most important part of my proposed change is to rename the methods to match those in the Java 8 equivalents so that we don't need overloads or additional methods (nor even default methods). Renaming the class/interface names is not strictly required, but IMO aligns our concepts with those in Java 8 and makes them a bit easier to grasp for new users.

If someone sees a flaw in my logic, please say so.

[ijuma]

Reading your message, I think you are referring to source compatibility, not binary compatibility right? I was talking about binary compatibility. Maybe we're OK with just offering source compatibility, but it's important to make that clear when we say that we are compatible.

[guozhangwang]

I thought it can be binary compatible as well?

http://wiki.eclipse.org/Evolving_Java-based_APIs_2#Evolving_API_interfaces_-_API_methods

[ijuma]

Guozhang, can you please quote what makes you say that?

Change type of a formal parameter   -   Breaks compatibility
Change result type (including void) -   Breaks compatibility

[rhauch]

@ijuma, from the Java SE 8 Language Specification, "inserting new class or interface types in the type hierarchy" will be binary compatible. That means that adding the Java 8 interfaces (e.g., BiPredicate) as super interfaces of our's (e.g., ...streams.Predicate) will be binary compatible.

However, you are right that changing the method signature (e.g., KStreams.filter(Predicate<K,V>) to filter(BiPredicate<K,V>)) will not be binary compatible. So rather than change the method signature, we can easily achieve binary compatibility by overloading the methods that take these types (e.g., in KStreams methods we can add default methods filter(BiPredicate<K,V>) and filterOut(BiPredicate<K,V>) that delegate to the existing methods, or vice versa). Also, the places where they're used are not functional interfaces, so adding the method should not be a problem -- even if they were functional interfaces, we can always add default methods to keep the interfaces functional.

The result is that the new Kafka version that will use Java 8 will be binary compatible: anyone trying to upgrade to the new Kafka version with or without recompiling would be fine, as they'd use the existing interfaces and methods, while those writing code against the newer Kafka version could use either the standard or Kafka functional interfaces. (BTW, this if we go this route, we should deprecate the older interfaces and older methods, giving some guidance as to which overloaded methods we want people to use.)

[ijuma]

@rhauch, right, this is exactly the issue I am outlining. You are now suggesting overloading, which is I mentioned in the original message is problematic:

"API design is hard. It was hard before, it has gotten harder now. With Java 8, if any of your API methods’ arguments are a functional interface, think twice about overloading that API method. And once you’ve concluded to proceed with overloading, think again, a third time whether this is really a good idea."
http://blog.jooq.org/2015/01/29/you-will-regret-applying-overloading-with-lambdas/

[ijuma]

Having said that, it would be good to verify our exact case, if we use overloads, does it cause inference issues when using them with lambdas?

[rhauch]

@ijuma, good catch. I tried making the changes locally (including jumping to Java 8), and overloading the methods does make using lambda's ambiguous. So, unless there's another way to do this, it looks like we're "stuck" with our interfaces -- which is perfectly fine. :-) They'll become functional interfaces when Kafka requires Java 8, and users will be able to use lambdas.

Then the question is: do we still want to make the changes in this PR? Personally, I think it is better to at least align with the names and concepts of standard Java 8 API, but that is completely a personal preference.

@ijuma, @guozhangwang, others: Thoughts?

[ijuma]

Thanks for trying it. I think the changes are still worth doing. I just wanted to make sure we understood our options going forward.

[guozhangwang]

@ijuma Thanks for pointing out the ambiguity issue, I have not considered this before. Currently we do not have two methods that takes the same number of interface functions yet, but I agree that moving forward we might.

I think we can still move forward with the name alignment just to keep the door open for now.

[guozhangwang]

LGTM, could you rebase on the latest trunk?

[rhauch]

@guozhangwang, rebased as requested, successfully retested the branch locally, and pushed the new commits.

[guozhangwang]

@rhauch Just realized there is another commit that gets conflicted with this one again while trying to push.. Could you pull again?

[rhauch]

@guozhangwang, rebased on trunk, successfully ran build locally, and updated this PR.

[guozhangwang]

"implements ProcessorSupplier<String, String>" with String, String for typesafe check.

[rhauch]

ack

[rhauch]

@guozhangwang, incorporated comments, successfully built locally, and updated PR.

[guozhangwang]

LGTM, thanks for the patch!