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

[TEST] Switch to persistent settings in java tests #78562

Merged
merged 15 commits into from
Oct 13, 2021
Merged

[TEST] Switch to persistent settings in java tests #78562

merged 15 commits into from
Oct 13, 2021

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Oct 1, 2021

We are deprecating transient cluster settings in 7.16.0, so this PR
changes various uses of transient settings in Java tests to use
persistent cluster settings instead.

Relates to #49540

Nikola Grcevski added 2 commits October 1, 2021 11:18
Migrate to persistent settings in Java tests
93bbc7b 
We are deprecating transient settings, therefore this
PR changes uses of transient cluster settings to
persistent cluster settings.
Merge branch 'master' into tests/persistent_settigns_java_tests
9ae1004
@grcevski grcevski added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label >deprecation v8.0.0 Team:Core/Infra Meta label for core/infra team auto-backport Automatically create backport pull requests when merged v7.16.0 labels Oct 1, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@grcevski grcevski requested a review from rjernst October 1, 2021 17:59
rjernst
rjernst requested changes Oct 5, 2021
View reviewed changes
Copy link
Member

@rjernst rjernst 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 in general for tests that were unnecessarily using transient settings, but for tests of the settings infrastructure itself I think we need to keep the tests, in some form.

client/rest-high-level/src/test/java/org/elasticsearch/client/ClusterClientIT.java Outdated

public class ClusterClientIT extends ESRestHighLevelClientTestCase {

public void testClusterPutSettings() throws IOException {
final String transientSettingKey = RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey();
Copy link
Member

Choose a reason for hiding this comment

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

We should keep some direct test(s) for transient settings, we’ll just need to handle the deprecation warning. They can go in dedicated test methods though, so they are easily removed in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to leave few of these inside the existing methods since they test combinations of persistent and transient settings used together. All of those that were pure transient settings usage are parameterized to try both transient and persistent.

server/src/internalClusterTest/java/org/elasticsearch/cluster/settings/ClusterSettingsIT.java
@@ -51,11 +50,11 @@ public void testClusterNonExistingSettingsUpdate() {
try {
client().admin().cluster()
.prepareUpdateSettings()
.setTransientSettings(Settings.builder().put(key1, value1).build())
Copy link
Member

Choose a reason for hiding this comment

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

This is another test case where we may want to keep some dedicated tested for transient settings.

server/src/internalClusterTest/java/org/elasticsearch/common/settings/UpgradeSettingsIT.java Outdated
@@ -82,10 +81,6 @@ public void testUpgradePersistentSettingsOnUpdate() {
runUpgradeSettingsOnUpdateTest((settings, builder) -> builder.setPersistentSettings(settings), Metadata::persistentSettings);
}

Copy link
Member

Choose a reason for hiding this comment

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

We should keep this test but handle the deprecation warning.

server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java
assertEquals("missing required setting [cluster.acc.test.user] for setting [cluster.acc.test.pw]", iae.getMessage());
client().admin().cluster().prepareUpdateSettings().setPersistentSettings(Settings.builder()
.put("cluster.acc.test.pw", "asdf")
.put("cluster.acc.test.user", "asdf")).get();

iae = expectThrows(IllegalArgumentException.class, () ->
Copy link
Member

Choose a reason for hiding this comment

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

Another test we should keep but separate.

@grcevski
Copy link
Contributor Author

grcevski commented Oct 5, 2021

This looks good in general for tests that were unnecessarily using transient settings, but for tests of the settings infrastructure itself I think we need to keep the tests, in some form.

Sounds good! I'll bring back the transient settings in those places and rework it a bit so we can handle the warning.

@grcevski grcevski requested a review from rjernst October 5, 2021 21:42
@grcevski
Copy link
Contributor Author

grcevski commented Oct 5, 2021

I brought back those tests and extracted where I could to use both transient and persistent settings.

rjernst
rjernst approved these changes Oct 12, 2021
View reviewed changes
Copy link
Member

@rjernst rjernst 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. A couple last minor questions

server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterStateDiffIT.java Outdated
} else {
return Metadata.builder(metadata).transientSettings(randomSettings(metadata.transientSettings())).build();
}
private Metadata metadataSettings(Metadata metadata) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the method name change? This is still calling randomSettings internally right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point, it was doing double random before :). I'll change it back.

docs/java-rest/high-level/cluster/put_settings.asciidoc Outdated
@@ -26,8 +26,7 @@ At least one setting to be updated must be provided:
--------------------------------------------------
include-tagged::{doc-tests-file}[{api}-request-cluster-settings]
--------------------------------------------------
<1> Sets the transient settings to be applied
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the docs stay for transient settings until we actually remove the feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yeah I wasn't sure what should we do here. I thought if we are deprecating them we shouldn't be advertising them as an available option anymore, but I'll bring it back and perhaps I'll label the usage as deprecated where needed.

@grcevski grcevski merged commit b2c5fd3 into elastic:master Oct 13, 2021
grcevski added a commit to grcevski/elasticsearch that referenced this pull request Oct 13, 2021
@grcevski
[TEST] Switch to persistent settings in java tests (elastic#78562)
2cceff3 
Migrate to persistent cluster settings in Java tests

We are deprecating transient settings, therefore this
PR changes uses of transient cluster settings to
persistent cluster settings.
@grcevski grcevski mentioned this pull request Oct 13, 2021
Merged
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
✅ 7.x

@grcevski grcevski mentioned this pull request Oct 13, 2021
Merged
grcevski added a commit that referenced this pull request Oct 14, 2021
@grcevski
[7.x] [TEST] Switch to persistent settings in java tests (#78562) (#7…
a7ae031 
…9103)

Migrate to persistent cluster settings in Java tests

We are deprecating transient settings, therefore this
PR changes uses of transient cluster settings to
persistent cluster settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >deprecation Team:Core/Infra Meta label for core/infra team >test Issues or PRs that are addressing/adding tests v7.16.0 v8.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grcevski @elasticmachine @elasticsearchmachine @rjernst @jakelandis