Skip to content

Clarify autoscaling feature flag registration #54427

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.

Sign up for GitHub

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 2 commits into from
Mar 31, 2020
Merged

Clarify autoscaling feature flag registration #54427

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a168389
Clarify autoscaling feature flag registration
jasontedor Mar 30, 2020
c73694d
Merge branch 'master' into autoscaling-feature-flag-registration-clar…
elasticmachine Mar 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions x-pack/plugin/autoscaling/src/main/java/org/elasticsearch/xpack/autoscaling/Autoscaling.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
*/
public class Autoscaling extends Plugin implements ActionPlugin {

private static final boolean AUTOSCALING_FEATURE_FLAG_REGISTERED;
private static final Boolean AUTOSCALING_FEATURE_FLAG_REGISTERED;

static {
final String property = System.getProperty("es.autoscaling_feature_flag_registered");
Expand All @@ -41,8 +41,10 @@ public class Autoscaling extends Plugin implements ActionPlugin {
}
if ("true".equals(property)) {
AUTOSCALING_FEATURE_FLAG_REGISTERED = true;
} else if ("false".equals(property) || property == null) {
} else if ("false".equals(property)) {
AUTOSCALING_FEATURE_FLAG_REGISTERED = false;
} else if (property == null) {
AUTOSCALING_FEATURE_FLAG_REGISTERED = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest leaving it as a non-nullable boolean and saying this here.

Suggested change
AUTOSCALING_FEATURE_FLAG_REGISTERED = null;
AUTOSCALING_FEATURE_FLAG_REGISTERED = Build.CURRENT.isSnapshot();

Then this flag exactly reflects whether the feature flag is registered or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's two issues with that suggestion:

  • my intention is that the field is aligned with the value of the system property es.autoscaling_feature_flag_registered, it's merely a Boolean representation of the value of that system property
  • the intention of using Autoscaling#isSnapshot in determining whether or not to register the setting is so that it can be overridden in tests; if we tie whether or not to register the setting directly to Build#Current#isSnapshot as you suggest, we lose that ability

} else {
throw new IllegalArgumentException(
"expected es.autoscaling_feature_flag_registered to be unset or [true|false] but was [" + property + "]"
Expand All @@ -69,7 +71,7 @@ public Autoscaling(final Settings settings) {
*/
@Override
public List<Setting<?>> getSettings() {
if (isSnapshot() || AUTOSCALING_FEATURE_FLAG_REGISTERED) {
if (isSnapshot() || (AUTOSCALING_FEATURE_FLAG_REGISTERED != null && AUTOSCALING_FEATURE_FLAG_REGISTERED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With my previous suggestion, this simplifies to the following:

Suggested change
if (isSnapshot() || (AUTOSCALING_FEATURE_FLAG_REGISTERED != null && AUTOSCALING_FEATURE_FLAG_REGISTERED)) {
if (AUTOSCALING_FEATURE_FLAG_REGISTERED) {

return List.of(AUTOSCALING_ENABLED_SETTING);
} else {
return List.of();
Expand Down