-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Implement lifecycle on SimulatePipelineRequest
#117585
Implement lifecycle on SimulatePipelineRequest
#117585
Conversation
Rather than releasing the REST request body after computing the response, we can link the lifecycles of the REST and transport requests and release the REST request body sooner. Not that we expect these bodies to be particularly large in this case, but still it's a better pattern to follow.
Pinging @elastic/es-data-management (Team:Data Management) |
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
@@ -20,7 +21,7 @@ public class SimulatePipelineRequestBuilder extends ActionRequestBuilder<Simulat | |||
* Create a new builder for {@link SimulatePipelineRequest}s | |||
*/ | |||
public SimulatePipelineRequestBuilder(ElasticsearchClient client, BytesReference source, XContentType xContentType) { | |||
super(client, SimulatePipelineAction.INSTANCE, new SimulatePipelineRequest(source, xContentType)); | |||
super(client, SimulatePipelineAction.INSTANCE, new SimulatePipelineRequest(ReleasableBytesReference.wrap(source), xContentType)); |
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.
nit: I'm thinking adding default method on BytesReference .toReleasable() would be more compact. There are many places where we wrap this.
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.
That would introduce a circular dependency between BytesReference
and ReleasableBytesReference
which suggests a design flaw. This isn't a very strong objection, this kind of circularity is not particularly strongly forbidden, but it does make me a little uneasy.
Rather than releasing the REST request body after computing the response, we can link the lifecycles of the REST and transport requests and release the REST request body sooner. Not that we expect these bodies to be particularly large in this case, but still it's a better pattern to follow.
Rather than releasing the REST request body after computing the response, we can link the lifecycles of the REST and transport requests and release the REST request body sooner. Not that we expect these bodies to be particularly large in this case, but still it's a better pattern to follow.
Rather than releasing the REST request body after computing the
response, we can link the lifecycles of the REST and transport requests
and release the REST request body sooner. Not that we expect these
bodies to be particularly large in this case, but still it's a better
pattern to follow.