Making changes to BulkRequest to enable component template substitutions in the simulate ingest API #112957
Conversation
…ons in the simulate ingest API
masseyke
added
>enhancement
:Data Management/Ingest Node
|
Hi @masseyke, I've created a changelog YAML for you. |
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
…:masseyke/elasticsearch into add-new-BulkRequest-methods-for-simulate
| public BulkRequest shallowClone() { | ||
| BulkRequest bulkRequest = new BulkRequest(); | ||
| bulkRequest.setRefreshPolicy(getRefreshPolicy()); | ||
| bulkRequest.waitForActiveShards(waitForActiveShards()); | ||
| bulkRequest.timeout(timeout()); | ||
| return bulkRequest; |
There was a problem hiding this comment.
BulkRequest has many other things than only the refresh policy, active shards, and timeout. Are we going to copy the rest of them also? Should the javadoc reflect what's being copied?
There was a problem hiding this comment.
I went his way because this is what BulkRequestModifier did (this code was moved out of there). I wavered on whether I ought to add the other fields or not, and wound up just repeating and testing the previous behavior. Any thoughts?
There was a problem hiding this comment.
I think just documenting it in the javadoc is fine
There was a problem hiding this comment.
I wound up adding the rest of the fields just in case (I don't think they'd intentionally been left out -- I think they were just added after the BulkRequestModifier method had been written).
| Map<String, AliasMetadata> aliases = null; | ||
| DataStreamLifecycle lifecycle = null; |
There was a problem hiding this comment.
Is this going to cause problems with it having unconsumed xcontent? Should we be throwing an exception for all other keys other than mappings and settings to show the user that those cannot be simulated? Or should we keep the leniency?
There was a problem hiding this comment.
For example, if we're ignoring eventual data_stream contents, then eventually from #98877 which wants to:
Figure out how to specify tsdb settings in component templates. For example index.routing_path can be specified in a composable index template if data stream template' index_mode is set to time_series.
Then we may need the whole thing for validation? This may be too premature, I'm just curious right now. Would it make sense to just load the whole thing into a Template from the JSON?
There was a problem hiding this comment.
Yeah I guess there's no harm in just using it all. Plus it simplifies my code. At this point though, I'm working with a Map -- think I ought to just pass it through all the way as xcontent? It's not really performance-sensitive since this is simulate code. And it's convenient for the sake of testing to have these overrides as a Map.
There was a problem hiding this comment.
The benefit is that if ComponentTemplate.parse ever changes to do some kind of validation, this would pick that up at least
…:masseyke/elasticsearch into add-new-BulkRequest-methods-for-simulate
| try (var parser = XContentHelper.mapToXContentParser(XContentParserConfiguration.EMPTY, rawTemplate)) { | ||
| componentTemplate = ComponentTemplate.parse(parser); | ||
| } catch (IOException e) { | ||
| throw new AssertionError(e); |
There was a problem hiding this comment.
I don't think we want to throw an assertion error here, as that would kill the ES node, we can probably throw an exception that will fail the simulation though?
There was a problem hiding this comment.
Oops. This was not supposed to be an AssertionError!
💔 Backport failed
You can use sqren/backport to manually backport by running |
…ons in the simulate ingest API (elastic#112957)
…ons in the simulate ingest API (elastic#112957)
3 participants
This is the first of a handful of PRs that will result in the addition of component template substitutions support to the simulate ingest API. See the draft PR at #112762 for the full idea.
This PR adds two methods to the BulkRequest API that will be used by the component template substitution code --
getComponentTemplateSubstitutions()andshallowClone(). Both will be used in a future update to TransportAbstractBulkAction.The next PR will add the transport changes to take advantage of these changes, and the final one will add the full documentation and rest-level testing (this PR updates RestSimulateIngestAction to pass the new SimulateBulkRequest constructor arg in order to compile, but there is no actual additional functionality at the rest layer).