Skip to content

[ML] Ensure immutability of MlMetadata#31957

Merged
dimitris-athanasiou merged 5 commits intoelastic:masterfrom
dimitris-athanasiou:ensure-immutability-of-ml-metadata
Jul 12, 2018
Merged

[ML] Ensure immutability of MlMetadata#31957
dimitris-athanasiou merged 5 commits intoelastic:masterfrom
dimitris-athanasiou:ensure-immutability-of-ml-metadata

Conversation

@dimitris-athanasiou
Copy link
Copy Markdown
Contributor

@dimitris-athanasiou dimitris-athanasiou commented Jul 11, 2018

The test failure in #31916 revealed that updating
rules on a job was modifying the detectors list
in-place. That meant the old cluster state and the
updated cluster state had no difference and thus the
change was not propagated to non-master nodes.

This commit fixes that and also reviews all of ML
metadata in order to ensure immutability.

Closes #31916

@dimitris-athanasiou
[ML] Ensure immutability of MlMetadata
319f5af 
The test failure in elastic#31916 revealed that updating
rules on a job was modifying the detectors list
in-place. That meant the old cluster state and the
updated cluster state had no difference and thus the
change was not propagated to non-master nodes.

This commit fixes that and also reviews all of ML
metadata in order to ensure immutability.

Closes elastic#31916
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jul 11, 2018

Pinging @elastic/ml-core

davidkyle
davidkyle reviewed Jul 11, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

Looks good. I would like to add more tests if possible perhaps in AnalysisConfigTests::mutateInstance as if detector rules where changed there we might have caught the bug

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java
updatedJob.getAnalysisConfig().getDetectors().get(detectorUpdate.getDetectorIndex()).getRules());
Detector updatedDetector = updatedJob.getAnalysisConfig().getDetectors().get(detectorUpdate.getDetectorIndex());
assertNotNull(updatedDetector);
assertEquals(detectorUpdate.getDescription(), updatedDetector.getDetectorDescription());
Copy link
Copy Markdown
Member

@davidkyle davidkyle Jul 11, 2018

Choose a reason for hiding this comment

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

Can you also assert that the original job has not been changed

Copy link
Copy Markdown
Contributor Author

@dimitris-athanasiou dimitris-athanasiou Jul 11, 2018

Choose a reason for hiding this comment

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

Done

droberts195
droberts195 reviewed Jul 11, 2018
View reviewed changes
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/datafeed/DatafeedConfig.java Outdated
this.scrollSize = scrollSize;
this.chunkingConfig = chunkingConfig;
this.headers = Objects.requireNonNull(headers);
this.headers = headers == null ? null : Collections.unmodifiableMap(headers);
Copy link
Copy Markdown

@droberts195 droberts195 Jul 11, 2018

Choose a reason for hiding this comment

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

headers should still not be allowed to be null.

Copy link
Copy Markdown
Contributor Author

@dimitris-athanasiou dimitris-athanasiou Jul 11, 2018

Choose a reason for hiding this comment

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

Done

@dimitris-athanasiou
Copy link
Copy Markdown
Contributor Author

dimitris-athanasiou commented Jul 11, 2018

I have added tests that attempt to ensure immutability of jobs/datafeeds, at least with regard to updatable properties. Could you have a look at those and let me know if you think they're worth it?

droberts195
droberts195 approved these changes Jul 12, 2018
View reviewed changes
Copy link
Copy Markdown

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@dimitris-athanasiou dimitris-athanasiou merged commit 2cfe703 into elastic:master Jul 12, 2018
@dimitris-athanasiou dimitris-athanasiou deleted the ensure-immutability-of-ml-metadata branch July 12, 2018 10:49
dimitris-athanasiou added a commit to dimitris-athanasiou/elasticsearch that referenced this pull request Jul 12, 2018
@dimitris-athanasiou
[6.x][ML] Ensure immutability of MlMetadata (elastic#31957)
e2c95ae 
The test failure in elastic#31916 revealed that updating
rules on a job was modifying the detectors list
in-place. That meant the old cluster state and the
updated cluster state had no difference and thus the
change was not propagated to non-master nodes.

This commit fixes that and also reviews all of ML
metadata in order to ensure immutability.

Closes elastic#31916
@dimitris-athanasiou dimitris-athanasiou mentioned this pull request Jul 12, 2018
Merged
dnhatn added a commit that referenced this pull request Jul 12, 2018
@dnhatn
Merge branch 'master' into ccr
b1f5d36 
* master:
  [TEST] Mute SlackMessageTests.testTemplateRender
  Docs: Explain closing the high level client
  [ML] Re-enable memory limit integration tests (#31328)
  [test] disable packaging tests for suse boxes
  Add nio transport to security plugin (#31942)
  XContentTests : Insert random fields at random positions (#30867)
  Force execution of fetch tasks (#31974)
  Fix unreachable error condition in AmazonS3Fixture (#32005)
  Tests: Fix SearchFieldsIT.testDocValueFields (#31995)
  Add Expected Reciprocal Rank metric (#31891)
  [ML] Get ForecastRequestStats doc in RestoreModelSnapshotIT (#31973)
  SQL: Add support for single parameter text manipulating functions (#31874)
  [ML] Ensure immutability of MlMetadata (#31957)
  Tests: Mute SearchFieldsIT.testDocValueFields()
  muted tests due to #31940
  Work around reported problem in eclipse (#31960)
  Move build integration tests out of :buildSrc project (#31961)
  Tests: Remove use of joda time in some tests (#31922)
  [Test] Reactive 3rd party tests on CI (#31919)
  SQL: Support for escape sequences (#31884)
  SQL: HAVING clause should accept only aggregates (#31872)
  Docs: fix typo in datehistogram (#31972)
  Switch url repository rest tests to new style requests (#31944)
  Switch reindex tests to new style requests (#31941)
  Docs: Added note about cloud service to installation and getting started
  [DOCS] Removes alternative docker pull example (#31934)
  Add Snapshots Status API to High Level Rest Client (#31515)
  ingest: date_index_name processor template resolution (#31841)
  Test: fix null failure in watcher test (#31968)
  Switch test framework to new style requests (#31939)
  Switch low level rest tests to new style Requests (#31938)
  Switch high level rest tests to new style requests (#31937)
  [ML] Mute test failing due to Java 11 date time format parsing bug (#31899)
  [TEST] Mute SlackMessageTests.testTemplateRender
  Fix assertIngestDocument wrongfully passing (#31913)
  Remove unused reference to filePermissionsCache (#31923)
  rolling upgrade should use a replica to prevent relocations while running a scroll
  HLREST: Bundle the x-pack protocol project (#31904)
  Increase logging level for testStressMaybeFlush
  Added lenient flag for synonym token filter (#31484)
  [X-Pack] Beats centralized management: security role + licensing (#30520)
  HLRest: Move xPackInfo() to xPack().info() (#31905)
  Docs: add security delete role to api call table (#31907)
  [test] port archive distribution packaging tests (#31314)
  Watcher: Slack message empty text (#31596)
  [ML] Mute failing DetectionRulesIT.testCondition() test
  Fix broken NaN check in MovingFunctions#stdDev() (#31888)
  Date: Add DateFormatters class that uses java.time (#31856)
  [ML] Switch native QA tests to a 3 node cluster (#31757)
  Change trappy float comparison (#31889)
  Fix building AD URL from domain name (#31849)
  Add opaque_id to audit logging (#31878)
  re-enable backcompat tests
  add support for is_write_index in put-alias body parsing (#31674)
  Improve release notes script (#31833)
  [DOCS] Fix broken link in painless example
  Handle missing values in painless (#30975)
  Remove the ability to index or query context suggestions without context (#31007)
  Ingest: Enable Templated Fieldnames in Rename (#31690)
  [Docs] Fix typo in the Rollup API Quick Reference (#31855)
  Ingest: Add ignore_missing option to RemoveProc (#31693)
  Add template config for Beat state to X-Pack Monitoring (#31809)
  Watcher: Add ssl.trust email account setting (#31684)
  Remove link to oss-MSI (#31844)
  Painless: Restructure Definition/Whitelist (#31879)
  HLREST: Add x-pack-info API (#31870)
@dimitris-athanasiou dimitris-athanasiou mentioned this pull request Jul 13, 2018
Merged
dimitris-athanasiou added a commit that referenced this pull request Jul 13, 2018
@dimitris-athanasiou
[ML] Updating detectors should not affect existing job (#32040)
eae6fed 
This is a light backport of #31957 for 6.3.
@DaveCTurner DaveCTurner mentioned this pull request Dec 7, 2018
Closed
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidkyle davidkyle davidkyle left review comments

+1 more reviewer

@droberts195 droberts195 droberts195 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>bug :ml Machine learning >non-issue v6.4.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ML] [CI] DetectionRulesIT.testCondition failure

5 participants

@dimitris-athanasiou @elasticmachine @davidkyle @droberts195 @jimczi