Skip to content

Conversation

@dakrone
Copy link
Member

@dakrone dakrone commented Jun 17, 2021

This converts the system property feature flag 'es.shutdown_feature_flag_enabled' to a regular
non-dynamic node setting. This setting can only be set to 'true' on a snapshot build of
Elasticsearch (not a release build).

Relates to #70338

This converts the system property feature flag '`es.shutdown_feature_flag_enabled`' to a regular
non-dynamic node setting. This setting can only be set to 'true' on a snapshot build of
Elasticsearch (not a release build).

Relates to elastic#70338
@dakrone dakrone added v8.0.0 :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown v7.14.0 labels Jun 17, 2021
@dakrone dakrone requested a review from gwbrown June 17, 2021 20:51
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jun 17, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

Left one minor suggestion. My only concern is: Are we sure that the tests will pass in release builds? Or do we need something like assumeTrue to prevent the integration tests from running? If we're good on that front, I'm good to merge this.

(admittedly depending on timing we may get rid of the feature flag before the next release is cut, but I'd rather be on the safe side)

@dakrone
Copy link
Member Author

dakrone commented Jun 17, 2021

Are we sure that the tests will pass in release builds? Or do we need something like assumeTrue to prevent the integration tests from running?

That's a good point. I think this might cause ES not to start for integration tests also, since we set it to true in build.gradle in a few places. I will investigate this.

@dakrone
Copy link
Member Author

dakrone commented Jun 21, 2021

@elasticmachine update branch

@dakrone
Copy link
Member Author

dakrone commented Jun 21, 2021

@gwbrown I think this should do the trick, can you take a look?

@dakrone dakrone requested a review from gwbrown June 21, 2021 22:05
Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

LGTM once CI is happy.

My only question is: Why don't we need assumeTrue or similar in NodeShutdownIT? I'd assume that would fail unless the feature flag is enabled too.

(If it turns out we do, no need for another round of review, that's a very minimal change)

@dakrone
Copy link
Member Author

dakrone commented Jun 21, 2021

Why don't we need assumeTrue or similar in NodeShutdownIT? I'd assume that would fail unless the feature flag is enabled too.

Yep, I think we need it there too, I'll add it to all those tests

@dakrone dakrone merged commit b566abc into elastic:master Jun 22, 2021
dakrone added a commit to dakrone/elasticsearch that referenced this pull request Jun 22, 2021
…c#74267)

This converts the system property feature flag 'es.shutdown_feature_flag_enabled' to a regular
non-dynamic node setting. This setting can only be set to 'true' on a snapshot build of
Elasticsearch (not a release build).

Relates to elastic#70338
dakrone added a commit that referenced this pull request Jun 22, 2021
…74267) (#74446)

This converts the system property feature flag 'es.shutdown_feature_flag_enabled' to a regular
non-dynamic node setting. This setting can only be set to 'true' on a snapshot build of
Elasticsearch (not a release build).

Relates to #70338
@probakowski
Copy link
Contributor

probakowski commented Jul 30, 2021

@dakrone I've marked it as non-issue, let me know if you think it should be something else

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown >non-issue Team:Core/Infra Meta label for core/infra team v7.14.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants