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

Replace ingest pipeline woraround in tsdb track #374

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

martijnvg
Copy link
Member

The workaround is replaced by a workaround that uses fixed start / end time range in the template.

This way there is no need to update document's @timestamp value before indexing with an ingest pipeline.
The @timestamp field values as they are can directly be used. This is beneficial because this doesn't
add noice of an ingest pipeline to the challenge. Other tsdb challenges don't use a pipeline.

This should allow for running with ingest_mode of value data_stream by default. Also in rally nightlies.

@martijnvg martijnvg requested a review from pquentin February 21, 2023 08:04
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks!

Can you please apply your index-template.json changes to index.json too?

Also, is this possible thanks to improvements in Elasticsearch? What is the latest Elasticsearch branch where this will work?

Comment on lines 25 to 27
"_source": {
"mode": {{ p_source_mode | tojson }}
},
Copy link
Member

Choose a reason for hiding this comment

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

Removing those lines is not necessary since #375.

And we have #377 to rely on Elasticsearch defaults. That said, we want to keep this to allow testing synthetic source with standard indices, which is tested in our nightly benchmarks.

Comment on lines 61 to 65
{%- if ingest_mode is defined and ingest_mode == "data_stream" %}
"index": "k8s",
{%- else %}
"index": "tsdb",
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of this 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.

This is a mistake. I will revert this. This search is ran when running the downsample challenge and in that case the tsdb-1d index exists.

@@ -74,7 +78,11 @@
{
"name": "date-histo-entire-range",
"operation-type": "search",
{%- if ingest_mode is defined and ingest_mode == "data_stream" %}
Copy link
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 otherwise when running with ingest_mode data_stream this search returns no results.

@martijnvg
Copy link
Member Author

Can you please apply your index-template.json changes to index.json too?

Done.

Also, is this possible thanks to improvements in Elasticsearch? What is the latest Elasticsearch branch where this will work?

Yes, tsdb indices use synthetic source by default. Starting from 8.7.0.

@pquentin
Copy link
Member

Also, is this possible thanks to improvements in Elasticsearch? What is the latest Elasticsearch branch where this will work?

Yes, tsdb indices use synthetic source by default. Starting from 8.7.0.

To be clear, "this" was referring to the fact that we can now remove the ingest pipeline. time_series start_time/end_time were introduced in 8.4 I think? I was wondering if something more recent allowed removing that pipeline. I'm assuming it's not thanks to the synthetic source default since it was possible to configure synthetic source with TSDB before 8.7.

Anyway, that's only my curiosity. The changes look great now, thank you!

@martijnvg
Copy link
Member Author

I'm assuming it's not thanks to the synthetic source default since it was possible to configure synthetic source with TSDB before 8.7.

Yes, this is why it can't be back ported beyond 8.7

@martijnvg martijnvg merged commit fb4388b into elastic:master Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants