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

Expose params to toXContentChunked as well as per-chunk #91771

Merged

Conversation

DaveCTurner
Copy link
Contributor

Some responses change shape depending on the supplied params, and/or parse certain details out of params. By passing the params to toXContentChunked we can adjust the shape of the returned iterator and/or avoid duplicate parsing effort in ways that are not possible today where params is only made available to each leaf ToXContent object.

Some responses change shape depending on the supplied `params`, and/or
parse certain details out of `params`. By passing the `params` to
`toXContentChunked` we can adjust the shape of the returned iterator
and/or avoid duplicate parsing effort in ways that are not possible
today where `params` is only made available to each leaf `ToXContent`
object.
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Nov 21, 2022
@DaveCTurner
Copy link
Contributor Author

TBH I think we could also pass in the builder once at the top level and then just have a Stream<Runnable> (maybe replacing Runnable with some more specific type for the context). Maybe that'd be too captureful? In any case this is enough for things like the cluster state API.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rjernst
Copy link
Member

rjernst commented Nov 22, 2022

Maybe that'd be too captureful? In any case this is enough for things like the cluster state API.

I would keep it simple. What you have already in this PR seems fine.

@original-brownbear
Copy link
Member

TBH I think we could also pass in the builder once at the top level and then just have a Stream (maybe replacing Runnable with some more specific type for the context). Maybe that'd be too captureful?

Yea that's how I had it in the initial version of this and would still like to have it. Henning made me change it to what we have now ...

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this but I'd like the version where we just capture the params once even better if possible. We shouldn't have done it like we had in the first place lets go all the way to the correct solution IMO

@DaveCTurner
Copy link
Contributor Author

Thanks both.

I missed the conversation about preferring to stick with the ToXContent API for the leaves - @henningandersen are you still sure we shouldn't use something simpler?

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@henningandersen
Copy link
Contributor

I missed the conversation about preferring to stick with the ToXContent API for the leaves - @henningandersen are you still sure we shouldn't use something simpler?

The primary problem I were trying to address was that each ChunkedToXContent had to do complicated state tracking, which this pattern avoided.

I am happy with the shape of this PR as is.

@DaveCTurner DaveCTurner merged commit 908e951 into elastic:main Nov 24, 2022
@DaveCTurner DaveCTurner deleted the 2022-11-21-outer-chunking-params branch November 24, 2022 10:03
DaveCTurner added a commit that referenced this pull request Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >refactoring Team:Core/Infra Meta label for core/infra team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants