Skip to content
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

[ML] Update ML mappings upgrade test and extend to config index #61340

Merged

Conversation

droberts195
Copy link
Contributor

The ML mappings upgrade test had become useless as it was
checking a field that has been the same since 6.5. This
commit switches to a field that was changed in 7.9.

Additionally, the test only used to check the results index
mappings. This commit also adds checking for the config
index.

Relates #61157

The ML mappings upgrade test had become useless as it was
checking a field that has been the same since 6.5. This
commit switches to a field that was changed in 7.9.

Additionally, the test only used to check the results index
mappings.  This commit also adds checking for the config
index.

Relates elastic#61157
@droberts195 droberts195 added >test Issues or PRs that are addressing/adding tests :ml Machine learning v8.0.0 v7.10.0 v7.9.1 labels Aug 19, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@@ -105,8 +120,40 @@ private void assertUpgradedMappings() throws Exception {
assertNotNull(propertiesLevel);
// TODO: as the years go by, the field we assert on here should be changed
// to the most recent field we've added that is NOT of type "keyword"
Map<String, Object> fieldLevel = (Map<String, Object>) propertiesLevel.get("multi_bucket_impact");
assertEquals(Collections.singletonMap("type", "double"), fieldLevel);
Map<String, Object> objectLevel = (Map<String, Object>) propertiesLevel.get("model_size_stats");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could using XContentMapValues::extractValue method make this more concise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. I switched over to that.


Map<String, Object> responseLevel = entityAsMap(response);
assertNotNull(responseLevel);
Map<String, Object> indexLevel = (Map<String, Object>) responseLevel.get(".ml-config");
Copy link
Contributor

Choose a reason for hiding this comment

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

The same here, could XContentMapValues::extractValue be used instead of this sequence of gets and casts?

@Mpdreamz Mpdreamz added v7.9.2 and removed v7.9.1 labels Sep 1, 2020
Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

LGTM

@droberts195 droberts195 merged commit 0a135a3 into elastic:master Sep 2, 2020
@droberts195 droberts195 deleted the validate_mappings_after_upgrade branch September 2, 2020 07:37
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Sep 2, 2020
The ML mappings upgrade test had become useless as it was
checking a field that has been the same since 6.5. This
commit switches to a field that was changed in 7.9.

Additionally, the test only used to check the results index
mappings.  This commit also adds checking for the config
index.

Backport of elastic#61340
droberts195 added a commit to droberts195/elasticsearch that referenced this pull request Sep 2, 2020
The ML mappings upgrade test had become useless as it was
checking a field that has been the same since 6.5. This
commit switches to a field that was changed in 7.9.

Additionally, the test only used to check the results index
mappings.  This commit also adds checking for the config
index.

Backport of elastic#61340
droberts195 added a commit that referenced this pull request Sep 2, 2020
The ML mappings upgrade test had become useless as it was
checking a field that has been the same since 6.5. This
commit switches to a field that was changed in 7.9.

Additionally, the test only used to check the results index
mappings.  This commit also adds checking for the config
index.

Backport of #61340
droberts195 added a commit that referenced this pull request Sep 2, 2020
The ML mappings upgrade test had become useless as it was
checking a field that has been the same since 6.5. This
commit switches to a field that was changed in 7.9.

Additionally, the test only used to check the results index
mappings.  This commit also adds checking for the config
index.

Backport of #61340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml Machine learning >test Issues or PRs that are addressing/adding tests v7.9.2 v7.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants