-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Making transport changes to enable component template substitutions in the simulate ingest API #113063
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
Making transport changes to enable component template substitutions in the simulate ingest API #113063
Conversation
…n the simulate ingest API
Pinging @elastic/es-data-management (Team:Data Management) |
…mponent-templates
…mponent-templates
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.
LGTM, I left a few really minor comments
assert (bulkRequest instanceof SimulateBulkRequest) == false | ||
: "TransportBulkAction should never be called with a SimulateBulkRequest"; |
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 add an assert that there are no template substitutions here? I.e., that getComponentTemplateSubstitutions()
returns an empty map?
} | ||
|
||
Builder(Metadata metadata) { | ||
public Builder(Metadata metadata) { |
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 think we want this to remain protected, favoring .builder(otherMeta)
instead
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.
Oh good point! I had missed that that method existed already.
final DocWriteRequest<?> originalRequest, | ||
final IndexRequest indexRequest, | ||
final Metadata metadata, | ||
Map<String, ComponentTemplate> templateSubstitutions |
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 call this var componentTemplateSubstitutions
since that's the name we've used elsewhere? (and presumably we'll have to add one for index template substitutions eventually)
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.
Yes definitely. I missed that one when I renamed all the rest.
…mponent-templates
…mponent-templates
…mponent-templates
…mponent-templates
…mponent-templates
…n the simulate ingest API (elastic#113063)
This is the second of three 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 updates TransportAbstractBulkAction and TransportSimulateBulkAction to use the substitute component templates that were added to BulkRequest in #112957.
The next PR will finish the rest layer, add the full documentation, and add rest-level testing.