Skip to content
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

Conversation

DaveCTurner
Copy link
Contributor

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.
@DaveCTurner DaveCTurner added >non-issue :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v9.0.0 labels Nov 26, 2024
@DaveCTurner DaveCTurner requested a review from mhl-b November 26, 2024 18:32
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Nov 26, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Copy link
Contributor

@mhl-b mhl-b left a 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));
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@DaveCTurner DaveCTurner merged commit 6130fbb into elastic:main Nov 27, 2024
16 checks passed
@DaveCTurner DaveCTurner deleted the 2024/11/26/SimulatePipelineRequest-lifecycle branch November 27, 2024 08:17
cbuescher pushed a commit to cbuescher/elasticsearch that referenced this pull request Nov 27, 2024
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.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >non-issue Team:Data Management Meta label for data/management team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants