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

Add unit tests for indices.recovery.max_bytes_per_sec default values #83261

Merged

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Jan 28, 2022

indices.recovery.max_bytes_per_sec has a default value that depends on multiple criteria that are well documented but not unit tested. This pull request introduces unit tests that verifies the current behavior so that future changes like #82819 are less likely to break things.

@tlrx tlrx added >test Issues or PRs that are addressing/adding tests :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 v8.1.0 v7.17.1 labels Jan 28, 2022
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jan 28, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@tlrx
Copy link
Member Author

tlrx commented Jan 28, 2022

@elasticmachine run elasticsearch-ci/bwc

@tlrx tlrx requested a review from henningandersen January 28, 2022 14:18
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

final ByteSizeValue totalPhysicalMemory = new ByteSizeValue(OsProbe.getInstance().getTotalPhysicalMemorySize());
final ByteSizeValue totalPhysicalMemory = TOTAL_PHYSICAL_MEMORY_OVERRIDING_TEST_SETTING.exists(s)
? TOTAL_PHYSICAL_MEMORY_OVERRIDING_TEST_SETTING.get(s)
: new ByteSizeValue(OsProbe.getInstance().getTotalPhysicalMemorySize());
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead let this value be the default value of the setting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

if (JavaVersion.current().compareTo(JavaVersion.parse("14")) < 0) {
JavaVersion javaVersion = JAVA_VERSION_OVERRIDING_TEST_SETTING.exists(s)
? JAVA_VERSION_OVERRIDING_TEST_SETTING.get(s)
: JavaVersion.current();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead let this value be the default value of the setting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

final ByteSizeValue random = randomByteSizeValue();
assertThat(
"Data nodes that are not dedicated to cold/frozen should use the defined rate limit when set",
nodeRecoverySettings().withRole(randomFrom("data", "data_hot", "data_warm", "data_content"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we randomly add either data_cold or data_frozen (or both) to the set of roles here?

Copy link
Member Author

Choose a reason for hiding this comment

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

You spotted it :)

@tlrx tlrx added auto-backport-and-merge auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Jan 28, 2022
@elasticsearchmachine elasticsearchmachine merged commit 24856ee into elastic:master Jan 28, 2022
@tlrx
Copy link
Member Author

tlrx commented Jan 28, 2022

Thanks Ievgen & Henning

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Jan 28, 2022
…lastic#83261)

`indices.recovery.max_bytes_per_sec` has a default value that depends on
multiple criteria that are well documented but not unit tested. This
pull request introduces unit tests that verifies the current behavior so
that future changes like elastic#82819 are less likely to break things.
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

The backport operation could not be completed due to the following error:
An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 83261

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Jan 28, 2022
…lastic#83261)

`indices.recovery.max_bytes_per_sec` has a default value that depends on
multiple criteria that are well documented but not unit tested. This
pull request introduces unit tests that verifies the current behavior so
that future changes like elastic#82819 are less likely to break things.
tlrx added a commit that referenced this pull request Jan 31, 2022
…83261) (#83283)

`indices.recovery.max_bytes_per_sec` has a default value that depends on
multiple criteria that are well documented but not unit tested. This
pull request introduces unit tests that verifies the current behavior so
that future changes like #82819 are less likely to break things.

Backport of #83261
tlrx added a commit that referenced this pull request Jan 31, 2022
…83261) (#83289)

`indices.recovery.max_bytes_per_sec` has a default value that depends on
multiple criteria that are well documented but not unit tested. This
pull request introduces unit tests that verifies the current behavior so
that future changes like #82819 are less likely to break things.

Backport of #83261
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Jan 31, 2022
* upstream/master: (100 commits)
  Avoid duplicate _type fields in v7 compat layer (elastic#83239)
  Bump bundled JDK to 17.0.2+8 (elastic#83243)
  [DOCS] Correct header syntax (elastic#83275)
  Add unit tests for indices.recovery.max_bytes_per_sec default values (elastic#83261)
  [DOCS] Add note that write indices are not replicated (elastic#82997)
  Add notes on indexing to kNN search guide (elastic#83188)
  Fix get-snapshot-api :docs:integTest (elastic#83273)
  FilterPathBasedFilter support match fieldname with dot (elastic#83178)
  Fix compilation issues in example-plugins (elastic#83258)
  fix ClusterStateListener javadoc (elastic#83246)
  Speed up Building Indices Lookup in Metadata (elastic#83241)
  Mute whole suite for elastic#82502 (elastic#83252)
  Make PeerFinder log messages happier (elastic#83222)
  [Docs] Add supported _terms_enum field types (elastic#83244)
  Add an aggregator for IPv4 and IPv6 subnets (elastic#82410)
  [CI] Fix 70_time_series/default sort yaml test failures (elastic#83217)
  Update test-failure Issue Template to include "needs:triage" label elastic#83226
  Add an index->step cache to the PolicyStepsRegistry (elastic#82316)
  Improve support for joda datetime to java datetime transition in Painless (elastic#83099)
  Fix joda migration for week based methods in Painless (elastic#83232)
  ...

# Conflicts:
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/TransportRollupAction.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >test Issues or PRs that are addressing/adding tests v7.17.1 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants