[ML] Correct handling of config updates #2821
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job.
edsavage
changed the title
Avoid explicitly specifying the template arguments by relying on the class template argument deduction.
valeriy42
approved these changes
Mar 5, 2025
Comment on lines
-252
to
-264
| //! Return a JSON string representing the analysis config | ||
| const std::string& getAnalysisConfig(); | ||
|
|
||
| //! Reparse the detector configuration object from within a stored | ||
| //! string representing the analysis config object. | ||
| //! This is necessary to correctly reinitialise scoped rule objects | ||
| //! folowing an update of the fiter rules configuration. | ||
| bool reparseDetectorsFromStoredConfig(const std::string& analysisConfig); | ||
|
|
||
| void setConfig(const std::string& analysisConfigString) { | ||
| m_AnalysisConfigString = analysisConfigString; | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you please leave a comment in this PR on why do you remove those?
There was a problem hiding this comment.
The code to set, get and reparse analysis config was introduced as part of the work to upgrade from boost 1.77 to 1.83. That upgrade included a major rewrite of boost unordered containers. Unfortunately it broke behaviour that we had come to rely on related to updating analysis config during a jobs' active lifetime. As a workaround the analysis config was reset to its previous state after an update, while allowing filter rules config to be updated. This PR provides a correct fix for the filter rules update, which also allows the analysis config to be updated safely as well. Therefore any code related to resetting the analysis config to its previous state is no longer needed.
lib/api/CConfigUpdater.cc
Outdated
| json::object obj = doc.as_object(); | ||
| for (json::object obj = doc.as_object(); const auto& kv : obj) { | ||
| if (kv.key() == CAnomalyJobConfig::MODEL_PLOT_CONFIG) { | ||
| LOG_DEBUG(<< "Updating model plot config"); |
There was a problem hiding this comment.
Suggested change
| LOG_DEBUG(<< "Updating model plot config"); | |
| LOG_TRACE(<< "Updating model plot config"); |
|
edsavage
added a commit
to edsavage/ml-cpp
that referenced
this pull request
Mar 5, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job.
edsavage
added a commit
to edsavage/ml-cpp
that referenced
this pull request
Mar 5, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job.
edsavage
added a commit
to edsavage/ml-cpp
that referenced
this pull request
Mar 6, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job.
edsavage
added a commit
to edsavage/ml-cpp
that referenced
this pull request
Mar 6, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job.
This was referenced Mar 6, 2025
edsavage
added a commit
that referenced
this pull request
Mar 6, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job. Backports #2821
edsavage
added a commit
that referenced
this pull request
Mar 6, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job. Backports #2821
edsavage
added a commit
that referenced
this pull request
Mar 6, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job. Backports #2821
edsavage
added a commit
that referenced
this pull request
Mar 6, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job. Backports #2821
edsavage
added a commit
that referenced
this pull request
Mar 6, 2025
* Enable updating of custom rules * Correctly update filters (scoped rules) These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job. Backports #2821
edsavage
added a commit
to edsavage/elasticsearch
that referenced
this pull request
Mar 7, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
edsavage
added a commit
to edsavage/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
edsavage
added a commit
to elastic/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
edsavage
added a commit
to edsavage/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
edsavage
added a commit
to edsavage/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
edsavage
added a commit
to edsavage/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
edsavage
added a commit
to edsavage/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
edsavage
added a commit
to edsavage/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
elasticsearchmachine
pushed a commit
to elastic/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
elasticsearchmachine
pushed a commit
to elastic/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
elasticsearchmachine
pushed a commit
to elastic/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
elasticsearchmachine
pushed a commit
to elastic/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
elasticsearchmachine
pushed a commit
to elastic/elasticsearch
that referenced
this pull request
Mar 10, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
georgewallace
pushed a commit
to georgewallace/elasticsearch
that referenced
this pull request
Mar 11, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
albertzaharovits
pushed a commit
to albertzaharovits/elasticsearch
that referenced
this pull request
Mar 13, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
Mar 13, 2025
This PR makes a change to the existing Java REST test DetectionRulesIT.testCondition such that it updates detection rules for a running job. Previously it had relied on closing and re-opening the job for the update to take effect. Relates elastic/ml-cpp#2821
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes fix a bug whereby memory corruption was caused when updating filters for an open job. The bug had previously been masked by code that reset the job's analysis config to its original state after every config update - the downside of this was that custom rules could not be updated on the fly, only by stopping and restarting the job.
Closes #2741
Initially opening as a draft PR to see how it behaves against the ES integration tests.