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

Make the tsdb track compatible with Elasticsearch 7.17 #743

Merged

Conversation

salvatore-campagna
Copy link
Contributor

@salvatore-campagna salvatore-campagna commented Feb 13, 2025

Here we make sure we exclude things from the configuration which are incompatible with
Elasticsearch v7. This is required to be able to use the challenge and the dataset in an
upgrade test which indexes metrics data into Elasticsearch 7.17.17 in standard mode. In that version
some things like source.mode, synthetic source, time series aggregations and so on were not available.
Also we need to disable the downsampling and low latency challenges because:

  • downsampling requires a time series index as a source index, which is not available in Elasticsearch v7
  • the low latency challenge uses document IDs to fetch documents which are likely different in Elasticsearch v7

@salvatore-campagna salvatore-campagna self-assigned this Feb 13, 2025
tsdb/track.json Outdated
@@ -7,6 +7,8 @@
{% set p_recovery_poll_timeout = (recovery_poll_timeout | default("1m")) %}
{% set p_recovery_max_operations_count = (recovery_max_operations_count | default(16777216)) %}

{% set p_include_esql_queries = (include_esql_queries | default(build_flavor != "serverless")) %}
Copy link
Member

Choose a reason for hiding this comment

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

Why not just have the default to be true, the feature doesnt really have anything to do with serverless, plus that keeps the default behaviour of the track the same

@dnhatn dnhatn marked this pull request as ready for review February 13, 2025 20:19
@dnhatn dnhatn requested a review from gareth-ellis February 13, 2025 20:19
@dnhatn
Copy link
Member

dnhatn commented Feb 13, 2025

@gareth-ellis I was able to rally this track with ES 7.17 with these params:

{
  "force_merge_max_num_segments": 1,
  "bulk_indexing_clients": 16,
  "corpus": "split16",
  "include_esql_queries": false,
  "include_source_mode": false,
  "skip_running_tsdb_aggs": true,
  "enable_downsample_challenge": false,
  "enable_low_latency_challenge": false
}

Can you take another look? Thank you!

@gareth-ellis
Copy link
Member

This is being merged into main, and seems that tsdb downsample challenge now is incompatible with main -

Cannot run task [downsample-1m]: Request returned an error. Error type: api, Description: exception ({'error': {'root_cause': [{'type': 'exception', 'reason': 'Rollup requires setting [index.mode=time_series] for index [tsdb]'}], 'type': 'exception', 'reason': 'Rollup requires setting [index.mode=time_series] for index [tsdb]'}, 'status': 500}), HTTP Status: 500

@dnhatn dnhatn marked this pull request as draft February 13, 2025 22:46
@salvatore-campagna
Copy link
Contributor Author

salvatore-campagna commented Feb 14, 2025

This is being merged into main, and seems that tsdb downsample challenge now is incompatible with main -

Cannot run task [downsample-1m]: Request returned an error. Error type: api, Description: exception ({'error': {'root_cause': [{'type': 'exception', 'reason': 'Rollup requires setting [index.mode=time_series] for index [tsdb]'}], 'type': 'exception', 'reason': 'Rollup requires setting [index.mode=time_series] for index [tsdb]'}, 'status': 500}), HTTP Status: 500

Yes:

  1. downsampling was introduced in v8 and never backported
  2. downsampling requires the source index to be a TSDB index. TSDB is an index mode introduced in v8 and never backported (as it requires TSDB).

So the downsampling challenge is incompatible with Elasticsearch v7 and the CI tries to run all of the challenges including the downsampling one. That is why I added enable_downsample_challenge.

The other challenge low_latency requires using specific document IDs...which are likely different in v7...that is the reason why I introduced enable_low_latency_challenge.

Anyway in the CI both challenges are executed because the default value for both enable_downsample_challenge and enable_downsample_challenge is true. Will try setting the default to false...

@salvatore-campagna salvatore-campagna marked this pull request as ready for review February 14, 2025 10:42
@salvatore-campagna
Copy link
Contributor Author

@martijnvg should we merge this?

@salvatore-campagna salvatore-campagna changed the title Make the TSDB append_no_conflict challenge compatible with Elasticsearch 7.17 Make the tsdb track compatible with Elasticsearch 7.17 Feb 14, 2025
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

should we merge this?

Yes.

Copy link
Member

@gareth-ellis gareth-ellis left a comment

Choose a reason for hiding this comment

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

LGTM

@salvatore-campagna
Copy link
Contributor Author

I will wait on Monday to merge this, just to avoid nightly benchmark issues affecting the TSDB track during the weekend.

@salvatore-campagna
Copy link
Contributor Author

This is a sample esbench configuration which runs the TSDB challenge with Elasticsearch 7.17.27.

{
    "elasticsearch.version": "7.17.27",
    "track.name": "tsdb",
    "track.challenge": "append-no-conflicts",
    "on_error": "abort",
    "track.params": {
      "force_merge_max_num_segments": 1,
      "bulk_indexing_clients": 16,
      "corpus": "split16",
      "include_source_mode": false,
      "skip_running_tsdb_aggs": true,
      "enable_downsample_challenge": false,
      "enable_low_latency_challenge": false
    },
    "client.options": {
        "default": {
            "timeout": 3600
        }
    },
    "elasticsearch.preserve_install": true
  }

@salvatore-campagna salvatore-campagna merged commit 4406464 into elastic:master Feb 17, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants