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

Update rally-* index template settings to default to Elasticsearch defaults #1312

Merged
merged 10 commits into from
Aug 16, 2021

Conversation

b-deam
Copy link
Member

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

In #1309 we added
configurable shard and replica shard counts to rally.ini.
In that commit, the default number of replicas was set to 0,
which is trappy given that Elasticsearch defaults to 1 replica.

This commit changes the default behaviour to add 1 replica to
the rally-* indices by default.

See comment: #1312 (comment)

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

As discussed offline let's change the logic to default to None and only attempt to override the number of shards and number of replicas if they've been explicitly set.
Additionally we'll need to remove the explicit "number_of_shards":1 from the three template files, e.g. here. With these changes implemented, metric stores with a default rally.ini will use the Elasticsearch index defaults for the rally-* indices (i.e. 1p/1r) and allow the user to override any of the two settings as needed.

@b-deam
Copy link
Member Author

b-deam commented Aug 16, 2021

As discussed, I've reworked the PR to meet the new requirements we discussed above ^^

The number of primary and replica shards are still configurable via rally.ini, but instead of explicitly settings defaults via rally, we fallback and allow Elasticsearch to set the defaults. We 'inject' these values into the respective rally metrics/races/results templates if a user has specified either or both datastore.number_of_shards/datastore.number_of_replicas.

In addition, we also remove the hardcoded refresh_interval: 5s setting to allow us to take advantage of Elasticsearch's default refresh behaviour:

index.refresh_interval
How often to perform a refresh operation, which makes recent changes to the index visible to search. Defaults to 1s. Can be set to -1 to disable refresh. If this setting is not explicitly set, shards that haven’t seen search traffic for at least index.search.idle.after seconds will not receive background refreshes until they receive a search request. Searches that hit an idle shard where a refresh is pending will wait for the next background refresh (within 1s). This behavior aims to automatically optimize bulk indexing in the default case when no searches are performed. In order to opt out of this behavior an explicit value of 1s should set as the refresh interval.

@b-deam b-deam changed the title Set default number of replicas to 1 for rally-* indices Update rally-* index template settings to default to Elasticsearch defaults Aug 16, 2021
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.

Looks good, left a comment about making the docs about the default value more factual.

docs/configuration.rst Outdated Show resolved Hide resolved
esrally/metrics.py Show resolved Hide resolved
@b-deam
Copy link
Member Author

b-deam commented Aug 16, 2021

Alrighty - I think I've addressed the comments and added a couple new tests. Can you take a look?

@b-deam b-deam requested a review from dliappis August 16, 2021 08:34
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, but we need to print the right ini file, honoring custom ini file invocations too.

esrally/metrics.py Outdated Show resolved Hide resolved
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.

There's a small bug left.

esrally/metrics.py Outdated Show resolved Hide resolved
@dliappis dliappis self-requested a review August 16, 2021 12:31
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

@dliappis dliappis merged commit 9474c0d into elastic:master Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :Metrics How metrics are stored, calculated or aggregated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants