Skip to content

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

Merged
DaveCTurner merged 1 commit intoelastic:mainfrom
DaveCTurner:2022-11-21-outer-chunking-params
Nov 24, 2022
Merged

Expose params to toXContentChunked as well as per-chunk#91771
DaveCTurner merged 1 commit intoelastic:mainfrom
DaveCTurner:2022-11-21-outer-chunking-params

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

@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
Copy Markdown
Member 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
Copy Markdown
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
Copy Markdown
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