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

Add force_synthetic_source to GET #87536

Merged
merged 2 commits into from
Jun 9, 2022
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jun 8, 2022

This adds the option to force synthetic source to the GET API. See
#87068 for more discussion on why you'd want to do that - the short
version is to get an upper bound on the performance cost of using
synthetic source in GET.

This adds the option to force synthetic source to the GET API. See
 elastic#87068 for more discussion on why you'd want to do that - the short
version is to get an upper bound on the performance cost of using
synthetic source in GET.
@nik9000 nik9000 requested a review from romseygeek June 8, 2022 18:25
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 8, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@nik9000 nik9000 mentioned this pull request Jun 8, 2022
50 tasks
@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Jun 8, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/clients-team (Team:Clients)

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -116,7 +116,8 @@ protected GetResponse shardOperation(GetRequest request, ShardId shardId) throws
request.realtime(),
request.version(),
request.versionType(),
request.fetchSourceContext()
request.fetchSourceContext(),
request.isForceSyntheticSource()
Copy link
Contributor

Choose a reason for hiding this comment

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

This was making me think that maybe we should add forceSynthetic as a member on FetchSourceContext, but it looks as though that is used in loads of other places so it would end up being a much bigger change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I thought about it and didn't like.

item.version(),
item.versionType(),
item.fetchSourceContext(),
false
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be enabled in a follow-up, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It'll be request.isForceSyntheticSource() I think.

@nik9000 nik9000 merged commit a37edb7 into elastic:master Jun 9, 2022
@javanna javanna added the :Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. label Jun 24, 2022
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jun 24, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/CRUD A catch all label for issues around indexing, updating and getting a doc by id. Not search. >non-issue :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) Team:Clients Meta label for clients team Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants