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.

Sign up for GitHub

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

Jump to bottom

Backport of types removal for Put/Get index templates #38022

Merged
merged 15 commits into from
Feb 5, 2019
Merged

Backport of types removal for Put/Get index templates #38022

merged 15 commits into from
Feb 5, 2019

Conversation

markharwood
Copy link
Contributor

Backport of PR #37484

I'll keep this as 2 commits to simplify review:

  1. The 7.0 changes in Types removal - deprecate include_type_name with index templates #37484, unchanged
  2. The (hopefully) small number of tweaks to adjust for 6.7 deprecation policies.

@markharwood markharwood added WIP :Search Foundations/Mapping Index mappings, including merging and defining field types v6.7.0 labels Jan 30, 2019
@markharwood markharwood self-assigned this Jan 30, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

@markharwood markharwood force-pushed the fix/35190TemplateDepv6 branch from 5508dcd to e695d56 Compare January 30, 2019 18:11
jtibshirani
jtibshirani reviewed Jan 30, 2019
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a couple small comments. One thing to note is that I think we will need to update FullClusterRestartIT on master to account for the fact that that index template requests on 6.7 can now produce deprecation warnings. I didn't do this after merging the 'create index' PR into 6.x, and ended up causing some failures on master.

client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesRequestConverters.java Outdated Show resolved Hide resolved
...nt/rest-high-level/src/test/java/org/elasticsearch/client/IndicesRequestConvertersTests.java
putTemplateRequest.mapping("{ \"properties\": { \"field-" + ESTestCase.randomInt() +
"\" : { \"type\" : \"" + ESTestCase.randomFrom("text", "keyword") + "\" }}}", XContentType.JSON);
}
if (ESTestCase.randomBoolean()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need to add expectedParams.put(INCLUDE_TYPE_NAME_PARAMETER, "false") here.

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java Outdated Show resolved Hide resolved
@markharwood markharwood force-pushed the fix/35190TemplateDepv6 branch from e695d56 to ae13841 Compare January 31, 2019 11:12
@jtibshirani
Copy link
Contributor

It looks like some HLRC tests might need adjusting. It would also be good to update to the new deprecation messages in #38052. Other than that, this looks ready to me!

@markharwood markharwood force-pushed the fix/35190TemplateDepv6 branch from 1ab74fb to 49556fe Compare February 1, 2019 14:56
@markharwood
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@markharwood markharwood force-pushed the fix/35190TemplateDepv6 branch from 0fbff41 to 6fe5ebb Compare February 1, 2019 15:27
jtibshirani
jtibshirani approved these changes Feb 1, 2019
View reviewed changes
Copy link
Contributor

@jtibshirani jtibshirani 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 to me. Just re-posting a note I put on a previous review, since it is easy to lose track of: after this merges I think we will need to update FullClusterRestartIT on master to account for the fact that that index template requests on 6.7 can now produce deprecation warnings.

...er/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetIndexTemplateAction.java
private static final DeprecationLogger deprecationLogger = new DeprecationLogger(
LogManager.getLogger(RestGetIndexTemplateAction.class));
public static final String TYPES_DEPRECATION_MESSAGE = "[types removal] The response format of get index template requests will change"
+ " in the next major version. Please start using the `include_type_name` parameter set to `false` in the request to "
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably not worth another full build, but just in case you need to rebuild for another reason -- we decided not to use backticks in the messages.

@markharwood markharwood force-pushed the fix/35190TemplateDepv6 branch 2 times, most recently from 94f3e73 to 6a17e55 Compare February 4, 2019 15:24
@markharwood markharwood removed the WIP label Feb 4, 2019
@markharwood markharwood force-pushed the fix/35190TemplateDepv6 branch from 6a17e55 to 4de6429 Compare February 4, 2019 20:13
@markharwood
Types removal - deprecate include_type_name with index templates (ela…
a549026 
…stic#37484)

Added deprecation warnings for use of include_type_name in put/get index templates.
HLRC changes:
GetIndexTemplateRequest has a new client-side class which is a copy of server's GetIndexTemplateResponse but modified to be typeless.
PutIndexTemplateRequest has a new client-side counterpart which doesn't use types in the mappings
Relates to elastic#35190
@markharwood
For ease of review, this small commit replaces the previous commit’s …
4eed856 
…back ported 7.x changes with the 6.x-specific deprecation policies.

Hopefully this makes review easier by not lumping in with all the 7.x code
@markharwood
Compile error
cf1dc3b
@markharwood
Checkstyle violation
2965733
@markharwood
Change test warning expectations
afef58a
@markharwood
Changed test’s expectation for serialisation format
ac8d994
@markharwood
Unused imports
3c792af
@markharwood
Rectified the way include_type_name is passed (or not as in prior cas…
02d9a35 
…e). Felt a little trappy to have my =false setting ignored.
@markharwood
Removed trappy Params.withIncludeTypeName.
c420ab2 
Other fixes for review comments
@markharwood
Removed unused import
3a780de
@markharwood
Only supply include_type_name if there’s mappings
db29f38
@markharwood
6.x assumes include_type_name = true so need to ask for false in this…
8ad5c0f 
… test
@markharwood markharwood force-pushed the fix/35190TemplateDepv6 branch from 4de6429 to 3fa0f09 Compare February 4, 2019 22:28
@jtibshirani
Copy link
Contributor

@elasticmachine run elasticsearch-ci/2

@jtibshirani
Copy link
Contributor

@markharwood mentioned it would be fine if I merged this when we got a green build.

@jtibshirani jtibshirani merged commit ca21cb2 into elastic:6.x Feb 5, 2019
alpar-t added a commit that referenced this pull request Feb 5, 2019
@alpar-t
Revert "Backport of types removal for Put/Get index templates (#38022)…
5e9688b 
…" (#38427)

This reverts commit ca21cb2.
@cbuescher cbuescher mentioned this pull request Feb 5, 2019
Merged
@markharwood markharwood mentioned this pull request Feb 5, 2019
Merged
cbuescher pushed a commit that referenced this pull request Feb 5, 2019
Ignore type-removal warnings in XPackRestTestHelper (#38431)
ca47f68 
The backport of #38022 introduced types-deprecation warning for get/put template requests
that cause problems on tests master in mixed cluster scenarios. While these warnings are
caught and ignored in regular Rest tests, the get template requests in XPackRestTestHelper
were missed.

Closes #38412
@markharwood markharwood mentioned this pull request Feb 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtibshirani jtibshirani jtibshirani approved these changes

Assignees

@markharwood markharwood

Labels
backport :Search Foundations/Mapping Index mappings, including merging and defining field types v6.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@markharwood @elasticmachine @jtibshirani @alpar-t