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

Remove configuration for user-defined services SPI #15951

Merged
merged 4 commits into from Nov 27, 2019

Conversation

@mmedenjak
Copy link
Contributor

mmedenjak commented Nov 7, 2019

In 4.0, we removed support for adding custom services by moving the
NodeEngine and all services to internal packages. With this, we remove
the configuration part for user-defined services. We still allow adding
custom services through a private ConfigAccessor method which is useful
for some tests or for users wanting to use private API at their own risk.

Also did some JDK8 cleanup.

Fixes: #15424
EE: hazelcast/hazelcast-enterprise#3334

@mmedenjak mmedenjak added this to the 4.0 milestone Nov 7, 2019
@mmedenjak mmedenjak requested a review from hazelcast/clients as a code owner Nov 7, 2019
@mmedenjak mmedenjak self-assigned this Nov 7, 2019
@sancar
sancar approved these changes Nov 7, 2019
Copy link
Member

sancar left a comment

Ok for the client side changes

@vojtechtoman vojtechtoman self-requested a review Nov 11, 2019
@Override
public String toString() {
return "Config{"
+ "clusterName=" + clusterName
+ "configurationUrl=" + configurationUrl

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Nov 11, 2019

Contributor

Just thinking out loud: wouldn't it be more practical to simply return the XML/YAML serialization of the effective config here? I wonder how readable the output of this toString() implementation is (and also about its maintainability).

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Nov 27, 2019

Author Contributor

You might be right. Don't know. I accidentally noticed some properties were missing so I decided to update it. Maybe I should have just ignored it :)

Seriously, we might use the ConfigXmlGenerator to generate the XML as I don't think we have YAML serialization. But I'm reluctant to do anything. I don't like this output as well as not liking outputting XML (it's not expected and this format, as much as it's verbose, is expected and parseable).

This comment has been minimized.

Copy link
@vojtechtoman

vojtechtoman Nov 27, 2019

Contributor

Yes, I was thinking the same about the expected format, verbosity etc. Let's pretend this discussion never took place.

This comment has been minimized.

Copy link
@mmedenjak

mmedenjak Nov 27, 2019

Author Contributor

Great, I wanted to suggest opening a GH issue and forgetting about it!

@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-private-services-config branch from dc59a1f to a376edc Nov 27, 2019
Matko Medenjak added 4 commits Nov 7, 2019
In 4.0, we removed support for adding custom services by moving the
NodeEngine and all services to internal packages. With this, we remove
the configuration part for user-defined services. We still allow adding
custom services through a private ConfigAccessor method which is useful
for some tests or for users wanting to use private API at their own risk.

Also did some JDK8 cleanup.
Matko Medenjak
@mmedenjak mmedenjak force-pushed the mmedenjak:4.0-private-services-config branch from 3549891 to 5729597 Nov 27, 2019
@mmedenjak mmedenjak merged commit 18387a9 into hazelcast:master Nov 27, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@mmedenjak mmedenjak deleted the mmedenjak:4.0-private-services-config branch Nov 27, 2019
@mmedenjak

This comment has been minimized.

Copy link
Contributor Author

mmedenjak commented Nov 27, 2019

Thank you for the reviews!

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