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

Deprecate support for chained multi-fields. #42330

Merged
merged 8 commits into from May 24, 2019

Conversation

Projects
None yet
3 participants
@jtibshirani
Copy link
Member

commented May 21, 2019

This PR contains a straight backport of #41926, and also updates the
migration documentation and deprecation info API for 7.x.

The code currently contains references to 7.2, but I will update these to 7.3
once we cut the branch over.

Deprecate support for chained multi-fields. (#41926)
We now issue a deprecation warning if a multi-field definition contains a
`[fields]` entry. This PR also simplifies the definition of
`MultiFieldParserContext`.

Addresses #41267.
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

@jtibshirani jtibshirani requested a review from talevy May 21, 2019

@jtibshirani jtibshirani force-pushed the jtibshirani:chained-multifields branch from 33b4003 to aa9cb58 May 21, 2019

@jtibshirani jtibshirani force-pushed the jtibshirani:chained-multifields branch from aa9cb58 to 22d00ed May 22, 2019

@gwbrown
Copy link
Contributor

left a comment

Thank you very much for implementing a deprecation check along with the warnings!

I left a couple comments asking for tweaks, but overall that part looks good. I don't see any obvious problems with the changes to the mapping code/tests, but I'm not very familiar with it.

if (issues.size() > 0) {
return new DeprecationIssue(DeprecationIssue.Level.CRITICAL,
"Multi-fields within multi-fields",
"https://www.elastic.co/guide/en/elasticsearch/reference/7.2/breaking-changes-7.2.html" +

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 22, 2019

Contributor

Historically, we've usually linked to the next major version breaking changes list (8.0 in this case), but this also has a few issues: Until the 8.0 branch is cut, the only way to link to these docs is by using master as the version in the URL, which 404s when 8.0 is actually released.

While having this in the breaking changes list for 7.2 and linking to it there would resolve this problem, I don't think it's how we've typically organized the breaking changes list, and might cause issues keeping up to date if we change the deprecation plan.

Basically there's a bunch of problems with how the breaking changes lists and links to them have work at the moment and there's no good options, all I'm pointing out here is that this is different from what we usually do.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani May 22, 2019

Author Member

Would you suggest I change this to master for now, for consistency with other deprecation issues we'll be adding? Then I guess we will update all of these links at once when the 8.0 branch is cut?

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 22, 2019

Contributor

Yes, I think that's what I would recommend for now, that way if we make a change to the docs to make the situation better it'll be easier to do all at once.

fieldLevelMappingIssue(indexMetaData, ((mappingMetaData, sourceAsMap) -> issues.addAll(
findInPropertiesRecursively(mappingMetaData.type(), sourceAsMap, IndexDeprecationChecks::containsChainedMultiFields))));
if (issues.size() > 0) {
return new DeprecationIssue(DeprecationIssue.Level.CRITICAL,

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 22, 2019

Contributor

Will 8.0 fail to open an index which has chained multi-fields that was created in 7.x? Usually we support all indices created in ($MAJOR-1), even if they use deprecated features.

If 8.0 will be able to open indices created in 7.x that have chained multi-fields, this should be Level.WARNING rather than Level.CRITICAL.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani May 22, 2019

Author Member

Thanks for pointing this out, I will make sure to update the 8.0 PR to still allow chained multi-fields on indices created prior to 8.0. With that change, I'll also be able to lower this to Level.WARNING.

@@ -110,6 +112,43 @@ public void testTooManyFieldsCheck() throws IOException {
assertEquals(0, withDefaultFieldIssues.size());
}

public void testChainedMultiFields() throws IOException {
XContentBuilder xContent = XContentFactory.jsonBuilder().startObject()

This comment has been minimized.

Copy link
@gwbrown

gwbrown May 22, 2019

Contributor

I'd like this test case to include at least one field that has a non-chained multi-field to verify that the warning message only contains the fields with a problem.

This comment has been minimized.

Copy link
@jtibshirani

jtibshirani May 22, 2019

Author Member

👍

@jtibshirani

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

@gwbrown this is now ready for another look.

@gwbrown
Copy link
Contributor

left a comment

LGTM, thanks @jtibshirani!

@jtibshirani jtibshirani merged commit 3a6c252 into elastic:7.x May 24, 2019

9 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@jtibshirani jtibshirani deleted the jtibshirani:chained-multifields branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.