Skip to content

Remove NioNotEnabledBootstrapCheck bootstrap check #28901

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

Merged
merged 4 commits into from
Mar 8, 2018

Conversation

Tim-Brooks
Copy link
Contributor

@Tim-Brooks Tim-Brooks commented Mar 5, 2018

This is related to #27260. This commit removes the bootstrap check that
prevents nio from being enabled.

This is related to elastic#27260. This commit adds a setting that will
override the nio not enabled bootstrap check. This will be necessary as
the bootstrap check would interfere with benchmarking or any
production-similar testing.
@@ -21,12 +21,23 @@

import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.bootstrap.BootstrapContext;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.util.concurrent.EsExecutors;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think this import should not be needed?


public class NioNotEnabledBootstrapCheck implements BootstrapCheck {

public static final Setting<Boolean> OVERRIDE_BOOTSTRAP =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh, I was not sure whether a system property or a setting would be a better choice but using a setting for this is consistent with what we did previously (see e.g. #20511).


public class NioNotEnabledBootstrapCheck implements BootstrapCheck {

public static final Setting<Boolean> OVERRIDE_BOOTSTRAP =
boolSetting("transport.nio.override_nio_bootstrap_check", false, Setting.Property.NodeScope);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively bootstrap.ignore_transport_nio_bootstrap_checks?

@jasontedor
Copy link
Member

Is this bootstrap check still needed? Can we remove it and only add it back if we decide NIO is not ready for production usage before 7.0.0?

@Tim-Brooks
Copy link
Contributor Author

@jasontedor I think that is a question for @s1monw. He asked me to add the bootstrap check in one of my past PRs.

@jasontedor
Copy link
Member

Sure thing let us wait for @s1monw, it's impotent though once there is a flag to skip it.

@s1monw
Copy link
Contributor

s1monw commented Mar 8, 2018

Is this bootstrap check still needed? Can we remove it and only add it back if we decide NIO is not ready for production usage before 7.0.0?

++ just remove it for now

@Tim-Brooks Tim-Brooks changed the title Add setting to override nio bootstrap check Remove nio non enabled bootstrap check Mar 8, 2018
@Tim-Brooks Tim-Brooks changed the title Remove nio non enabled bootstrap check Remove NioNotEnabledBootstrapCheck bootstrap check Mar 8, 2018
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Tim-Brooks Tim-Brooks merged commit 7d434c1 into elastic:master Mar 8, 2018
sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018
This is related to elastic#27260. This commit removes the bootstrap check that
prevents nio from being enabled.
@Tim-Brooks Tim-Brooks deleted the override_bootstrap branch December 10, 2018 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Network Http and internode communication implementations >non-issue v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants