Skip to content

Add force_synthetic_source to mget#87574

Merged
nik9000 merged 2 commits intoelastic:masterfrom
nik9000:synthetic_force_mget
Jun 22, 2022
Merged

Add force_synthetic_source to mget#87574
nik9000 merged 2 commits intoelastic:masterfrom
nik9000:synthetic_force_mget

Conversation

@nik9000
Copy link
Copy Markdown
Member

@nik9000 nik9000 commented Jun 9, 2022

This adds the option to force synthetic source to the MGET 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 MGET.

@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 9, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

@nik9000 nik9000 mentioned this pull request Jun 9, 2022
50 tasks
This adds the option to force synthetic source to the MGET 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 MGET.
@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Jun 9, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

@nik9000 nik9000 requested a review from romseygeek June 16, 2022 18:51
Copy link
Copy Markdown
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.

One question re the public API, but LGTM otherwise.

"description": "Should this request force synthetic _source? Use this to test if the mapping supports synthetic _source and to get a sense of the worst case performance. Fetches with this enabled will be slower the enabling synthetic source natively in the index.",
"visibility": "feature_flag",
"feature_flag": "es.index_mode_feature_flag_registered"
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can't remember what we agreed here, didn't we suggest that we wouldn't document this at all so that it didn't appear as part of the 'official' API, but that Kibana or whoever could still use it by setting params explicitly on the underlying request?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is required for the tests to run. But the clients team is not picking it up - I think because of the feature_flag, but I'm not sure. I've talked with them. And with the kibana folks - they can use it without changes to the APIs.

And, yeah, I'm not documenting this in the asciidoc files. It's a debugging tool and anyone with the wherewithal to use it is probably also following the repo.

@nik9000 nik9000 merged commit 463d46c into elastic:master Jun 22, 2022
@javanna javanna added the :Distributed/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 Meta label for distributed team. label Jun 24, 2022
@elasticmachine
Copy link
Copy Markdown
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/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 Meta label for distributed team. v8.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants