[TEST] Switch to persistent settings in java tests by grcevski · Pull Request #78562 · elastic/elasticsearch

grcevski · 2021-10-01T15:24:37Z

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

We are deprecating transient settings, therefore this PR changes uses of transient cluster settings to persistent cluster settings.

elasticmachine · 2021-10-01T15:24:39Z

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

rjernst

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.

rjernst · 2021-10-05T12:50:33Z

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

 public class ClusterClientIT extends ESRestHighLevelClientTestCase {

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


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.

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.

rjernst · 2021-10-05T12:56:21Z

server/src/internalClusterTest/java/org/elasticsearch/cluster/settings/ClusterSettingsIT.java

        try {
            client().admin().cluster()
                    .prepareUpdateSettings()
-                    .setTransientSettings(Settings.builder().put(key1, value1).build())


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

rjernst · 2021-10-05T12:57:23Z

server/src/internalClusterTest/java/org/elasticsearch/common/settings/UpgradeSettingsIT.java

@@ -82,10 +81,6 @@ public void testUpgradePersistentSettingsOnUpdate() {
        runUpgradeSettingsOnUpdateTest((settings, builder) -> builder.setPersistentSettings(settings), Metadata::persistentSettings);
    }



We should keep this test but handle the deprecation warning.

rjernst · 2021-10-05T12:58:02Z

server/src/internalClusterTest/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java

+            .put("cluster.acc.test.pw", "asdf")
+            .put("cluster.acc.test.user", "asdf")).get();

        iae = expectThrows(IllegalArgumentException.class, () ->


Another test we should keep but separate.

grcevski · 2021-10-05T13:43:27Z

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.

Revert back tests that test specifically for the transient settings functionality.

grcevski · 2021-10-05T21:42:58Z

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

rjernst

Looks good. A couple last minor questions

rjernst · 2021-10-12T21:23:51Z

server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterStateDiffIT.java

-        } else {
-            return Metadata.builder(metadata).transientSettings(randomSettings(metadata.transientSettings())).build();
-        }
+    private Metadata metadataSettings(Metadata metadata) {


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

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

rjernst · 2021-10-12T21:47:34Z

docs/java-rest/high-level/cluster/put_settings.asciidoc

 --------------------------------------------------
 include-tagged::{doc-tests-file}[{api}-request-cluster-settings]
 --------------------------------------------------
-<1> Sets the transient settings to be applied


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

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.

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.

elasticsearchmachine · 2021-10-13T20:25:46Z

💚 Backport successful

Status	Branch	Result
✅	7.x

…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.

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 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

Bring in one doc file to match test.

b9f5e5b

grcevski requested a review from rjernst October 1, 2021 17:59

rjernst requested changes Oct 5, 2021

View reviewed changes

Nikola Grcevski added 5 commits October 5, 2021 09:59

Merge branch 'master' into tests/persistent_settigns_java_tests

28de853

Revert back some of the tests

0d6174f

Revert back tests that test specifically for the transient settings functionality.

Improve tests to separate transient.

689e49a

Merge branch 'master' into tests/persistent_settigns_java_tests

83a308d

Fix exception label issue.

e90822a

grcevski requested a review from rjernst October 5, 2021 21:42

Merge branch 'master' into tests/persistent_settigns_java_tests

9c0be28

rjernst approved these changes Oct 12, 2021

View reviewed changes

Nikola Grcevski added 6 commits October 13, 2021 10:46

Merge branch 'master' into tests/persistent_settigns_java_tests

d169fe6

Merge master

71f1426

Revert method rename

c3a78c7

Revert docs change.

b882a10

Merge branch 'master' into tests/persistent_settigns_java_tests

b8cbc7a

Bring back true transient test.

178410c

grcevski merged commit b2c5fd3 into elastic:master Oct 13, 2021

grcevski mentioned this pull request Oct 13, 2021

[7.x] [TEST] Switch to persistent settings in java tests (#78562) #79103

Merged

grcevski mentioned this pull request Oct 13, 2021

Deprecation of transient cluster settings #78794

Merged

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

		@@ -82,10 +81,6 @@ public void testUpgradePersistentSettingsOnUpdate() {
		runUpgradeSettingsOnUpdateTest((settings, builder) -> builder.setPersistentSettings(settings), Metadata::persistentSettings);
		}

Conversation

grcevski commented Oct 1, 2021

Uh oh!

elasticmachine commented Oct 1, 2021

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grcevski commented Oct 5, 2021

Uh oh!

grcevski commented Oct 5, 2021

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Oct 13, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants