-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Enable write load constraint decider by default #135246
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
Enable write load constraint decider by default #135246
Conversation
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a question
if (WRITE_LOAD_DECIDER_ENABLED_FF.isEnabled()) { | ||
clusterSettings.initializeAndWatch(WRITE_LOAD_DECIDER_ENABLED_SETTING, status -> this.writeLoadDeciderStatus = status); | ||
} else { | ||
writeLoadDeciderStatus = WriteLoadDeciderStatus.DISABLED; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means we cannot test by overriding the setting in a serverless QA enviroment. Is this intentional? Or maybe we should set the default accordingly based on the feature flag and leave the dynamic update always enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! changed in 149b2ea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I thought we'd just have to restart the cluster the first time it was turned on (to enable the feature flag). But your suggested approach is much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
setWriteLoadDeciderEnablement( | ||
randomBoolean() | ||
? WriteLoadConstraintSettings.WriteLoadDeciderStatus.ENABLED | ||
: WriteLoadConstraintSettings.WriteLoadDeciderStatus.LOW_THRESHOLD_ONLY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would your helper method be appropriate for an explicit DISABLED settings update in the finally
block below, and in other tests?
IIUC, we'll still need the finally block's disable setting update, so that these tests will pass when the release build runs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ESSingleNodeTestCase
checks that there is no persistent metadata (including settings) left behind in teardown, so we need to clear these settings before the test ends.
We need to clear the settings, rather than setting them to a specific value. I've added a helper for that too in 9845c38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here are just to stop depending on the default being one way or another, because this will change depending on the feature flag.
public class WriteLoadConstraintSettings { | ||
|
||
private static final String SETTING_PREFIX = "cluster.routing.allocation.write_load_decider."; | ||
private static final FeatureFlag WRITE_LOAD_DECIDER_ENABLED_FF = new FeatureFlag("write_load_decider_enabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably get rid of 'enabled' in both names, as it's redundant, but doesn't really matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, improved in 917c617
Add a feature flag for the write load decider, and change the enabled setting to default to
ENABLED
. This will mean the decider and its infrastructure will be enabled for snapshot builds, but disabled in production and release builds.Relates: ES-12881