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.

Sign up for GitHub

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

Jump to bottom

Configure persistent datastore index settings #1310

Merged
merged 8 commits into from
Aug 12, 2021
Merged

Configure persistent datastore index settings #1310

merged 8 commits into from
Aug 12, 2021

Conversation

b-deam
Copy link
Member

@b-deam b-deam commented Aug 9, 2021

With this commit we add the ability for users to specify the number of
primary and replica shard counts for rally-* indices when using a
persistent metrics store through the configuration of reporting settings
in rally.ini.

Closes #1309

@b-deam b-deam added enhancement Improves the status quo :Metrics How metrics are stored, calculated or aggregated labels Aug 9, 2021
@b-deam b-deam added this to the 2.3.0 milestone Aug 9, 2021
@b-deam b-deam requested a review from dliappis August 9, 2021 03:30
@b-deam b-deam self-assigned this Aug 9, 2021
dliappis
dliappis reviewed Aug 9, 2021
View reviewed changes
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

This looks good and works fine in tests. Left a few comments.

esrally/config.py Outdated Show resolved Hide resolved
esrally/metrics.py Outdated Show resolved Hide resolved
docs/configuration.rst Show resolved Hide resolved
esrally/metrics.py Outdated Show resolved Hide resolved
esrally/metrics.py Outdated Show resolved Hide resolved
@b-deam b-deam requested a review from dliappis August 10, 2021 06:47
@b-deam
Copy link
Member Author

b-deam commented Aug 10, 2021

I made the changes - but given that it shouldn't be possible for an instance of InMemoryMetricsStore to have an index_template_provider_class attribute I wasn't sure on how to test this.

@dliappis
Copy link
Contributor

I made the changes - but given that it shouldn't be possible for an instance of InMemoryMetricsStore to have an index_template_provider_class attribute I wasn't sure on how to test this.

Additional settings like datastore.host simply get ignored when datastore.type != elasticsearch so I am not sure what you are aiming to test?

@b-deam
Copy link
Member Author

b-deam commented Aug 11, 2021

I made the changes - but given that it shouldn't be possible for an instance of InMemoryMetricsStore to have an index_template_provider_class attribute I wasn't sure on how to test this.

Additional settings like datastore.host simply get ignored when datastore.type != elasticsearch so I am not sure what you are aiming to test?

Right - I guess I want to ensure that if any changes are made to the InMemoryMetricsStore in so that if it ever implements/utilises a index_template_provider_class, then I guess we should make sure that we catch that in unit tests because all of the logic around is currently based on the assumption that it will only ever be executed in the context of a EsMetricsStore.

@dliappis
Copy link
Contributor

if any changes are made to the InMemoryMetricsStore in so that if it ever implements/utilises a index_template_provider_class

I don't think this will ever happen. @danielmitterdorfer do you feel we need a unit test for this unlikely scenario?

@danielmitterdorfer
Copy link
Member

I don't think this will ever happen. @danielmitterdorfer do you feel we need a unit test for this unlikely scenario?

No, that will not happen. The index template provider is designed to be used only with an EsMetricsStore.

@b-deam
Copy link
Member Author

b-deam commented Aug 12, 2021

Thanks @danielmitterdorfer, @dliappis.

I had the misconception that 'In-memory' == local metrics store - so it's obvious now that we'd never implement a template provider 🤦 .

I removed the unit test for the InMemoryMetricsStore. Will merge after CI passes.

dliappis
dliappis approved these changes Aug 12, 2021
View reviewed changes
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM

@b-deam b-deam merged commit 27b97b5 into elastic:master Aug 12, 2021
dliappis pushed a commit that referenced this pull request Aug 16, 2021
@b-deam
Update rally-* index template settings to default to Elasticsearch de…
9474c0d 
…faults (#1312)

This commit changes the defaults for the number of shards and replicas used in the Elasticsearch datastore first introduced in #1310:
instead of hardcoding them -- when unset -- to `1p/0r`, we rely on ES defaults[1] and only attempt to override these index settings if they've
been explicitly specified in the `ini` file.

Additionally, we also remove the hardcoded `refresh_interval: 5s` setting, to allow us to take advantage of Elasticsearch's default refresh behavior ([2], [3]).

\[1\]: https://www.elastic.co/guide/en/elasticsearch/reference/current/index-modules.html#_static_index_settings
\[2\]: https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-refresh.html#refresh-api-desc
\[3\]: https://www.elastic.co/guide/en/elasticsearch/reference/current/index-modules.html#dynamic-index-settings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dliappis dliappis dliappis approved these changes

Assignees

@b-deam b-deam

Labels
enhancement Improves the status quo :Metrics How metrics are stored, calculated or aggregated
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

Allow parametrization of Rally Elasticsearch datastore index settings
3 participants
@b-deam @dliappis @danielmitterdorfer