Make certain ML node settings dynamic (#33565) #33961
Conversation
|
Pinging @elastic/ml-core |
dimitris-athanasiou
left a comment
dimitris-athanasiou
left a comment
There was a problem hiding this comment.
Looks good. Left a comment about how we access the max anomaly records setting.
| } | ||
|
|
||
| autodetectBuilder.build(); | ||
| autodetectBuilder.build(maxAnomalyRecords); |
There was a problem hiding this comment.
As we are creating a new AutodetectBuilder each time, I think we can pass it the settings in each constructor. We currently pass it the stored settings that could be out of date. But we can pass it instead clusterService.settings(). I am not sure what the difference between clusterService.settings() and clusterService.clusterSettings() is, but if we figure that out I'm sure we can make it work. This would mean we don't need to have a listener in AutodetectProcessManager for the setting plus we can keep the build() method without any arguments.
|
|
||
| static String maxAnomalyRecordsArg(int maxAnomalyRecords) { | ||
| return "--maxAnomalyRecords=" + maxAnomalyRecords; | ||
| static String maxAnomalyRecordsArg(Settings settings) { |
There was a problem hiding this comment.
Cool! Only thing is we now could make this method non-static and not have to pass in the settings. There are possibly more methods in there that could be non-static. Not necessary to do this in this PR though, only if you feel like it :-)
| public static final Setting<Integer> MAX_ANOMALY_RECORDS_SETTING = Setting.intSetting("max.anomaly.records", DEFAULT_MAX_NUM_RECORDS, | ||
| Setting.Property.NodeScope); | ||
| Setting.Property.NodeScope, Setting.Property.Deprecated); | ||
| public static final Setting<Integer> MAX_ANOMALY_RECORDS_SETTING_DYNAMIC = Setting.intSetting( |
There was a problem hiding this comment.
It would be good to add a comment that although this is now dynamic, updates only apply to newly opened jobs, not already running jobs.
I don't think this setting will be changed often enough that this is worth fixing. (At the moment the setting is completely undocumented, so it's unlikely anyone will be changing it.) But it's good to have a comment pointing out the limitation so that if we ever do decide to document it then the limitation will hopefully get documented too.
4 participants
There are certain settings that could be made dynamic. This addresses that and marks old settings as deprecated in preparation to v7.
DONT_PERSIST_MODEL_STATE_SETTINGis the only other setting I could find, but I do not think that should be made dynamic as the state may already be persisted and the pipeline already created.Another possible setting is
DEFAULT_BASE_PERSIST_INTERVAL, however, I am not sure end users really care about this as replaying the job with just 3 hours of data to recover is pretty simple. @droberts195 @dimitris-athanasiou let me know what you think.closes #33565