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

Fixed fluent interface of config classes #11107

Conversation

@Donnerbart
Copy link
Contributor

Donnerbart commented Aug 15, 2017

Added missing return this statements to setter methods.

The fluent interface of our config classes was broken in several places. This makes it hard to write good test code, since you could not chain all setter methods of a config.

This should not break any compatibility, since the return type is not part of the method signature in Java. The compatibility tests are also still passing.

@Donnerbart Donnerbart self-assigned this Aug 15, 2017
@Donnerbart Donnerbart force-pushed the Donnerbart:fixFluentInterfaceOfConfigClasses branch 2 times, most recently from fa480a5 to e6cd6a3 Aug 15, 2017
Added missing `return this` statements to setter methods.
@Donnerbart Donnerbart force-pushed the Donnerbart:fixFluentInterfaceOfConfigClasses branch from e6cd6a3 to 34cfe8a Aug 15, 2017
@pveentjer

This comment has been minimized.

Copy link
Member

pveentjer commented Aug 17, 2017

This can cause class compatibility issues if these methods are already exposed in 3.8 or before. On bytecode level the return type is part of the signature to determine the method to call for nvocation. So if we are adding a return type to a void method, then code compiled against the void method will run into a NoSuchMethodError.

@Donnerbart

This comment has been minimized.

Copy link
Contributor Author

Donnerbart commented Aug 17, 2017

Peter is correct, I confused compile time with runtime compatibility. According to our wiki a compile time change is forbidden in patch releases, but allowed for minor releases. So we can do this in 3.9 or 3.10

screenshot from 2017-08-17 17-53-25

@jerrinot @tombujok WDYT? Should we move this to 3.10 or is it fine to fix this in 3.9?

@jerrinot

This comment has been minimized.

Copy link
Contributor

jerrinot commented Aug 17, 2017

@Donnerbart: Hazelcast 4.0

@Donnerbart

This comment has been minimized.

Copy link
Contributor Author

Donnerbart commented Aug 17, 2017

Why not 3.10? We cannot move everything to 4.0 and the compatibility requirements would not be broken for a minor release. You could even consider it as bug, that you cannot use the fluent interface with all config methods. It completely breaks the code flow, when you write a chained config and suddenly you are stopped by a void setter. This is not "Hazelcast should be easy to use".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.