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

made RestApiConfig not null value by default #15981

Merged
merged 2 commits into from Nov 19, 2019

Conversation

@alex-dukhno
Copy link
Contributor

alex-dukhno commented Nov 11, 2019

Fix for #15968

Also removed these group properties:

    /**	
     * Enables or disables MEMCACHE protocol on members.	
     *	
     * @see com.hazelcast.config.MemcacheProtocolConfig	
     */	
    public static final HazelcastProperty MEMCACHE_ENABLED	
            = new HazelcastProperty("hazelcast.memcache.enabled", false);	
    /**	
     * Enables or disables REST API on members.	
     *	
     * @see com.hazelcast.config.RestEndpointGroup	
     * @see com.hazelcast.config.RestApiConfig	
     */	
    public static final HazelcastProperty REST_ENABLED	
            = new HazelcastProperty("hazelcast.rest.enabled", false);	

    /**	
     * Enables or disables HTTP health check API on members.	
     *	
     * @see com.hazelcast.config.RestEndpointGroup#HEALTH_CHECK	
     */	
    public static final HazelcastProperty HTTP_HEALTHCHECK_ENABLED	
            = new HazelcastProperty("hazelcast.http.healthcheck.enabled", false);

@vbekiaris @tkountis could you guys have a look please?

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

devOpsHazelcast commented Nov 11, 2019

Can one of the admins verify this patch?

Copy link
Contributor

tkountis left a comment

Thanks @alex-dukhno.
Since you are there, mind initializing memcache config too?

@vbekiaris

This comment has been minimized.

Copy link
Contributor

vbekiaris commented Nov 13, 2019

In NodeIOService#initRestApiConfig:

boolean isRestConfigPresent = isAdvancedNetwork
? config.getAdvancedNetworkConfig().getEndpointConfigs().get(EndpointQualifier.REST) != null
: restApiConfig != null;
if (isRestConfigPresent) {
// ensure the legacy Hazelcast group properties are not provided
ensurePropertyNotConfigured(properties, GroupProperty.REST_ENABLED);
ensurePropertyNotConfigured(properties, GroupProperty.HTTP_HEALTHCHECK_ENABLED);
}

So in order to make use of the legacy REST_ENABLED and HTTP_HEALTHCHECK_ENABLED config properties, with this PR one must explicitly initialize NetworkConfig#restApiConfig to null. I am not sure if those properties are meant to be removed or maintained in 4.0, maybe @kwart knows?

@kwart

This comment has been minimized.

Copy link
Contributor

kwart commented Nov 13, 2019

So in order to make use of the legacy REST_ENABLED and HTTP_HEALTHCHECK_ENABLED config properties, with this PR one must explicitly initialize NetworkConfig#restApiConfig to null. I am not sure if those properties are meant to be removed or maintained in 4.0, maybe @kwart knows?

The properties are deprecated, even they are not annotated with @Deprecated. We should remove them together with GroupProperty.MEMCACHE_ENABLED

if (checkAndLogPropertyDeprecated(properties, GroupProperty.REST_ENABLED)) {
restApiConfig.setEnabled(true);
restApiConfig.enableAllGroups();
}
if (checkAndLogPropertyDeprecated(properties, GroupProperty.HTTP_HEALTHCHECK_ENABLED)) {
restApiConfig.setEnabled(true);
restApiConfig.enableGroups(RestEndpointGroup.HEALTH_CHECK);
}

if (checkAndLogPropertyDeprecated(properties, GroupProperty.MEMCACHE_ENABLED)) {

@alex-dukhno

This comment has been minimized.

Copy link
Contributor Author

alex-dukhno commented Nov 13, 2019

Thanks @kwart, I will do it in this PR

@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Nov 14, 2019

We decided to undeprecate those properties because they are useful for container configuration - #15743
Quoting Rafal:

I see, I'd say in all this Docker and Kubernetes world, it's more convenient for users to configure with the command lines parameters (or JVM params or env var) than to make changes in the Hazelcast configuration
so every change like this makes things rather worse,
because if you want to run Hazelcast Docker image and include your custom configuration, then it's kind-of difficult

@tkountis

This comment has been minimized.

Copy link
Contributor

tkountis commented Nov 14, 2019

We probably need a better way of handling container configuration. It will be difficult to track props that we need or don’t based on this context. And also makes it kinda impossible to get away from the properties that nobody likes, even for simple flags like these.

@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Nov 14, 2019

We do need a better way, and I would love to see spring boot style config overrides from command line but until then, do we remove the properties and force users to use XML/YAML with config replacers?

@tkountis

This comment has been minimized.

Copy link
Contributor

tkountis commented Nov 14, 2019

IMO we should not start making exceptions, it can get really messy pretty fast. It took four people’s involvement in this tiny PR to get to this state.

@kwart

This comment has been minimized.

Copy link
Contributor

kwart commented Nov 14, 2019

I agree with Thomas.

Properties should not be used for switching on/off standard features where we have the typed config ready.

If the typed config is not sufficient for container/cloud scenarios, let's prioritize some better way of configuration (which the properties IMO are not).

@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Nov 14, 2019

https://media.giphy.com/media/v0eHX3n28wvoQ/source.gif

I realise how ugly this looks but I'm still reluctant to remove the properties. We should include @leszko and others into this decision to see how bad it will be and if it can be alleviated.

@leszko

This comment has been minimized.

Copy link
Contributor

leszko commented Nov 14, 2019

Guys, how about creating a PRD to make each property possible to override from command-line? With YAML Configuration already in place, that should not be that difficult.

It's like what they do in Helm Chart, you have a YAML configuration like:

hazelcast:
  image: hazelcast/hazelcast

Then, you can override each parameter of that configuration with

$ helm install --set hazelcast.image=leszko/hazelcast .

About this particular change, hmm, we already have a parameter that you can't set from the command line and we template it inside the Docker image. Technically, we could do the same for rest.enabled (and for other parameters).

@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Nov 14, 2019

Yes, exactly what I would like to see! I'll create the PRD. As for these properties... Yeah, I'm really sorry to see them go since as you said, it makes things harder. But for the sake of completeness and cleanliness, let's remove them :(

@tkountis tkountis self-requested a review Nov 14, 2019
@alex-dukhno alex-dukhno force-pushed the alex-dukhno:NonNull-default-RestApiConfig branch from f81b9ea to 462f000 Nov 14, 2019
@vbekiaris

This comment has been minimized.

Copy link
Contributor

vbekiaris commented Nov 15, 2019

also mvn -Pcheckstyle clean install reports:

13:12:07 [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder_2/hazelcast/src/test/java/com/hazelcast/internal/ascii/MemcachedTest.java:26:8: Unused import - com.hazelcast.spi.properties.GroupProperty. [UnusedImports]
@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Nov 15, 2019

Created the PRD so let's hope it gets into the next release :)

@leszko

This comment has been minimized.

Copy link
Contributor

leszko commented Nov 15, 2019

Created the PRD so let's hope it gets into the next release :)

Thanks @mmedenjak !

@devOpsHazelcast

This comment has been minimized.

Copy link
Contributor

devOpsHazelcast commented Nov 18, 2019

Can one of the admins verify this patch?

* make RestApiConfig and MemcacheProtocolConfig non null value by default
* remove REST_ENABLED, HTTP_HEALTHCHECK_ENABLED and MEMCACHE_ENABLED properties
@alex-dukhno alex-dukhno force-pushed the alex-dukhno:NonNull-default-RestApiConfig branch from 50db38c to dda3152 Nov 18, 2019
@alex-dukhno

This comment has been minimized.

Copy link
Contributor Author

alex-dukhno commented Nov 18, 2019

@vbekiaris the test is fixed and I squashed all commits into one. Thank you

@kwart

This comment has been minimized.

Copy link
Contributor

kwart commented Nov 18, 2019

run-lab-run

@hazelcast hazelcast deleted a comment from vbekiaris Nov 18, 2019
@hazelcast hazelcast deleted a comment from vbekiaris Nov 18, 2019
Copy link
Contributor

vbekiaris left a comment

thanks @alex-dukhno

@leszko leszko mentioned this pull request Nov 18, 2019
}

/**
* Initializes {@link RestApiConfig} if not provided based on legacy group properties. Also checks (fails fast)
* if both the {@link RestApiConfig} and system properties are used.
* Initializes {@link RestApiConfig} if not provided based on legacy group properties.

This comment has been minimized.

Copy link
@tkountis

tkountis Nov 19, 2019

Contributor

Javadoc needs update, no props involved anymore.

This comment has been minimized.

Copy link
@alex-dukhno

alex-dukhno Nov 19, 2019

Author Contributor

Would you mind if I remove it entirely? As for me method body speaks for itself. Thanks

This comment has been minimized.

Copy link
@tkountis

tkountis Nov 19, 2019

Contributor

In that particular case I agree. Go for it.

This comment has been minimized.

Copy link
@alex-dukhno

alex-dukhno Nov 19, 2019

Author Contributor

Done. I believe it is ready for merge? Thanks

@tkountis

This comment has been minimized.

Copy link
Contributor

tkountis commented Nov 19, 2019

Yes it seems ready, once again thanks for the contribution!

@tkountis tkountis merged commit db2cbae into hazelcast:master Nov 19, 2019
@alex-dukhno alex-dukhno deleted the alex-dukhno:NonNull-default-RestApiConfig branch Nov 19, 2019
petrpleshachkov pushed a commit to petrpleshachkov/hazelcast that referenced this pull request Nov 22, 2019
Make RestApiConfig and MemcacheProtocolConfig non null values by default
and remove REST_ENABLED, HTTP_HEALTHCHECK_ENABLED and MEMCACHE_ENABLED properties
mmedenjak pushed a commit to mmedenjak/hazelcast-reference-manual that referenced this pull request Nov 26, 2019
- `hazelcast.rest.enabled`, `hazelcast.memcache.enabled` and
`hazelcast.http.healthcheck.enabled` were in fact removed with
hazelcast/hazelcast#15981
- fixed WAN replication configuration
- "group property" has been renamed to "cluster property"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.