Remove types from BulkRequest #46983
Conversation
romseygeek
added
:Distributed/CRUD
|
Pinging @elastic/es-distributed |
| */ | ||
| BulkRequestBuilder prepareBulk(@Nullable String globalIndex, @Nullable String globalType); | ||
| BulkRequestBuilder prepareBulk(@Nullable String globalIndex); |
There was a problem hiding this comment.
NB I will open a separate PR for 7x to deprecate the method that takes a global type here.
…ction' into types-removal/bulk-request-action
| @Nullable String defaultPipeline, boolean allowExplicitIndex, | ||
| XContentType xContentType, | ||
| Consumer<IndexRequest> indexRequestConsumer, | ||
| BiConsumer<IndexRequest, String> indexRequestConsumer, |
There was a problem hiding this comment.
This is a slightly hacky way of getting around the fact that the Monitoring Bulk action still uses the _type parameter in a bulk index line to set a type field. This action is slated for deprecation and removal at some point in the future; until then, seeing as the BulkParser no longer sets the type on its IndexRequest, we need to pass it separately to this consumer.
There was a problem hiding this comment.
I don't love that we need to maintain types in one place in the bulk action because of the monitoring bulk action. I'd like to try to find some way to avoid this whilst minimising the affect on the monitoring bulk actions API itself. This doesn't need to be solved in this PR but I'd like to try to solve it in the types removal for 8.0 work
| */ | ||
| public BulkRequestParser(boolean warnOnTypeUsage) { | ||
| this.warnOnTypeUsage = warnOnTypeUsage; | ||
| public BulkRequestParser(boolean errorOnType) { |
There was a problem hiding this comment.
Should we have a zero arg constructor that defaults errorOnType to true? That way there should be less places to update when we remove this parameter?
There was a problem hiding this comment.
This is only actually called from two locations - BulkRequest and MonitoringBulkRequest - so I'm not sure a zero-arg constructor actually helps much here.
| if (in.getVersion().before(Version.V_8_0_0)) { | ||
| in.readString(); | ||
| // can't make an assertion about type names here because too many tests still set their own | ||
| // types bypassing various checks |
There was a problem hiding this comment.
How many tests do this? I'm wondering if we need to make a separate change to make all the tests use the typeless APIs and backport it to 7.x so we can have these checks?
There was a problem hiding this comment.
I'll re-add it in and change the relevant tests.
There was a problem hiding this comment.
OK, so the reason that we can't assert the type here is indeed for backwards compatibility; it's a failure message, and there are certain circumstances in which a user can supply information which results in a different type being returned here: specifically, if a user tries to index documents with different type names, then we return an error message saying you can only have a single type, and the type field in this failure message will be set to one of those user-supplied names.
| @Nullable String defaultPipeline, boolean allowExplicitIndex, | ||
| XContentType xContentType, | ||
| Consumer<IndexRequest> indexRequestConsumer, | ||
| BiConsumer<IndexRequest, String> indexRequestConsumer, |
There was a problem hiding this comment.
I don't love that we need to maintain types in one place in the bulk action because of the monitoring bulk action. I'd like to try to find some way to avoid this whilst minimising the affect on the monitoring bulk actions API itself. This doesn't need to be solved in this PR but I'd like to try to solve it in the types removal for 8.0 work
|
The changes to |
This reverts commit 72977c4.
|
@elasticmachine update branch |
…ction' into types-removal/bulk-request-action
|
@elasticmachine update branch |
retrofits typed endpoint and type in request parsing the original types removal commit elastic#46983 relates elastic#51816
This commit removes types entirely from BulkRequest, both as a global
parameter and as individual entries on update/index/delete lines.
Relates to #41059