-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Remove type field from internal PutMappingRequest #48793
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
Remove type field from internal PutMappingRequest #48793
Conversation
|
Pinging @elastic/es-core-features (:Core/Features/Indices APIs) |
|
Pinging @elastic/es-search (:Search/Mapping) |
|
There are a number of changes here to just pass |
mayya-sharipova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romseygeek Thanks Alan! I left small comments, but overall LGTM
| return this; | ||
| @Override | ||
| public String toString() { | ||
| return "put-mapping"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be something else added (e.g. source of the request)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the history, I'm not sure why I added this in the first place, as it didn't have a toString() previously. I'll remove it.
| PutMappingRequest request = new PutMappingRequest(index); | ||
|
|
||
| String type = randomAlphaOfLength(5); | ||
| request.type(type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also make a type "_doc" on the line 135?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
| final long previousVersion = clusterService.state().metaData().index("test").getMappingVersion(); | ||
| final PutMappingRequest request = new PutMappingRequest(); | ||
| request.indices("test"); | ||
| request.type("type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also other tests in this file to use type _doc (e.g. line 167) or the intent is to do this in following PRs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan on doing this in subsequent PRs - for example, line 167 will need to change when we remove type from MapperService.merge
| indicesOptions = IndicesOptions.readIndicesOptions(in); | ||
| type = in.readOptionalString(); | ||
| if (in.getVersion().before(Version.V_8_0_0)) { | ||
| in.readOptionalString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could check the type and throw an IllegalArgumentException here as we've been doing in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
|
@elasticmachine update branch |
External API requests can no longer include types, so we have no need to pass this
information along in internal PutMappingClusterStateUpdateRequest objects, or on
PutMappingRequests used by the internal node client.
Relates to #41059