Deprecate settings in .yml and .json #24059

rjernst · 2017-04-12T06:07:51Z

This commit adds a deprecation warning when elasticsearch.yml or
elasticsearch.json is read during startup. It also fixes all docs
references, tests, and renames the default file that comes in our packages.

relates #19391

rjernst · 2017-04-12T06:12:25Z

One thing I am unsure about is how to handle packaging correctly with the change in name. I'm thinking we actually need to have something in the packaging scripts, otherwise I think the user will end up with both elasticsearch.yml and elasticsearch.yaml.

jasontedor

It looks great. You covered all the things I thought of. I left a few comments.

jasontedor · 2017-04-12T06:12:03Z

core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java

@@ -44,6 +47,9 @@

 public class InternalSettingsPreparer {

+    private static Logger logger = ESLoggerFactory.getLogger("settings");


Why name this logger against our convention? It's not even prefixed with "org.elasticsearch" but I think you should only pass InternalSettingsPreparer.class here.

I was trying to make the context in the log message clearer (that it had to do with settings) vs a user thinking "what is InternalSettingsPreparer". But I changed it as you suggested. If we don't want to allow using getLogger(String), we should remove it.

There are a few legitimate uses for this method, namely the loggers that we provide to Netty, and the naming for the circuit breaker loggers, as well as setting logging levels via the API (all we get there from the user input is the name of the logger, so we have to have a way to look up loggers by name).

jasontedor · 2017-04-12T06:13:51Z

core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java

@@ -112,6 +118,9 @@ public static Environment prepareEnvironment(Settings input, Terminal terminal,
            throw new SettingsException("multiple settings files found with suffixes: "
                + Strings.collectionToDelimitedString(foundSuffixes, ","));
        }
+        if (foundSuffixes.contains(".yml") || foundSuffixes.contains(".json")) {
+            deprecationLogger.deprecated("elasticsearch.yaml and elasticsearch.json are deprecated. Rename your configuration file to elasticsearch.yaml.");


Three-letter "yml" for the first instance of "yaml" here.

I think we can give a more specific message too: yml -> yaml only if yml, json -> yaml only if json instead of a general message covering both cases.

Oops, I wrote this before doing find replace. :)

Because we disallow multiple settings files (a few lines up), we are certain here that foundSuffixes contains at most one element. So we can write:

if (foundSuffixes.contains(".yml") || foundSuffixes.contains(".json")) { final DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(InternalSettingsPreparer.class)); deprecationLogger.deprecated( "elasticsearch[{}] is deprecated; rename your configuration file to elasticsearch.yaml", foundSuffixes.iterator().next()); }

I think this is clean, and let's us scope the logger locally so we can avoid the static fields.

jasontedor · 2017-04-12T06:14:52Z

core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java

@@ -44,6 +47,9 @@

 public class InternalSettingsPreparer {

+    private static Logger logger = ESLoggerFactory.getLogger("settings");
+    private static DeprecationLogger deprecationLogger = new DeprecationLogger(logger);


Also I wonder if these loggers should be initialized in the one place they are used?

That is, locally, and not even be a field here.

Since I split the message into two branches, I left these static.

See my suggestion for how I think that we can avoid this.

rjernst · 2017-04-26T21:49:00Z

@jasontedor Maybe we should separate adding the deprecation from changing the shipped file? Then we could only do the default file change in 6.0. Users would then get the deprecation warning in 5.5, and know that they need to rename their file before upgrading to 6.0.

This commit adds a deprecation warning when elasticsearch.yml or elasticsearch.json is read during startup. relates elastic#19391

rjernst · 2017-04-27T21:53:25Z

@jasontedor I pushed 129bc76 which simplies this commit to be just deprecation. I will cutover the docs and shipped config file in a followup to master only (with removal of support for anything but .yaml and helpful error message on the old names existing).

rjernst · 2017-05-02T15:47:33Z

@elasticmachine retest this please

@jasontedor ping

jasontedor

I left a comment.

jasontedor · 2017-05-02T16:06:29Z

core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java

@@ -107,6 +109,11 @@ public static Environment prepareEnvironment(Settings input, Terminal terminal,
            throw new SettingsException("multiple settings files found with suffixes: "
                + Strings.collectionToDelimitedString(foundSuffixes, ","));
        }
+        if (foundSuffixes.contains(".yml") || foundSuffixes.contains(".json")) {
+            final DeprecationLogger deprecationLogger = new DeprecationLogger(Loggers.getLogger(InternalSettingsPreparer.class));


Sadly, the logging here happens before we've fully configured logging. This means that it will appear in the console log with a default format, and before we've configured deprecation logging so it won't end up in the deprecation log on disk.

Good point. I pushed 7f796b0 to address this.

jasontedor · 2017-05-03T04:33:50Z

retest this please

rjernst · 2017-05-09T05:58:06Z

@elasticmachine retest this please

dliappis · 2017-05-09T13:55:07Z

jenkins test this please

rjernst · 2017-05-11T03:12:53Z

@jasontedor I finally got a clean CI run, can you look again?

jasontedor

LGTM.

This commit adds a deprecation warning when elasticsearch.yml or elasticsearch.json is read during startup. relates #19391

rjernst added :Core/Infra/Settings Settings infrastructure and APIs v5.4.0 v6.0.0-alpha1 labels Apr 12, 2017

jasontedor requested changes Apr 12, 2017

View reviewed changes

Settings: Deprecate settings in .yml and .json

129bc76

This commit adds a deprecation warning when elasticsearch.yml or elasticsearch.json is read during startup. relates elastic#19391

rjernst force-pushed the deprecate_yml branch from bb7d603 to 129bc76 Compare April 27, 2017 21:51

clintongormley added v5.4.1 and removed v5.4.0 labels Apr 28, 2017

rjernst added v5.5.0 and removed v5.4.1 labels May 2, 2017

jasontedor requested changes May 2, 2017

View reviewed changes

rjernst added 2 commits May 2, 2017 10:01

Merge branch 'master' into deprecate_yml

494867b

Add config extension to env and move deprecation to bootstrap

7f796b0

clintongormley added v6.0.0 and removed v6.0.0-alpha1 labels May 3, 2017

rjernst added 6 commits May 9, 2017 09:20

Merge branch 'master' into deprecate_yml

a327a20

Merge branch 'master' into deprecate_yml

5f58b46

fix hamcrest matcher

ce3f85f

Merge branch 'master' into deprecate_yml

5f3794a

Merge branch 'master' into deprecate_yml

9e79e15

Merge branch 'master' into deprecate_yml

476eec1

jasontedor approved these changes May 11, 2017

View reviewed changes

rjernst merged commit f477a64 into elastic:master May 11, 2017

rjernst deleted the deprecate_yml branch May 11, 2017 20:11

rjernst added a commit that referenced this pull request May 11, 2017

Settings: Deprecate settings in .yml and .json (#24059)

827bd08

This commit adds a deprecation warning when elasticsearch.yml or elasticsearch.json is read during startup. relates #19391

clintongormley added v6.0.0-alpha2 >deprecation and removed v6.0.0 labels Jun 6, 2017

clintongormley changed the title ~~Settings: Deprecate settings in .yml and .json~~ Deprecate settings in .yml and .json Jun 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate settings in .yml and .json #24059

Deprecate settings in .yml and .json #24059

rjernst commented Apr 12, 2017

rjernst commented Apr 12, 2017

jasontedor left a comment

jasontedor Apr 12, 2017

rjernst Apr 12, 2017

jasontedor Apr 12, 2017

jasontedor Apr 12, 2017

jasontedor Apr 12, 2017

rjernst Apr 12, 2017

jasontedor Apr 12, 2017

jasontedor Apr 12, 2017

jasontedor Apr 12, 2017

rjernst Apr 12, 2017

jasontedor Apr 12, 2017

rjernst commented Apr 26, 2017

rjernst commented Apr 27, 2017

rjernst commented May 2, 2017

jasontedor left a comment

jasontedor May 2, 2017

rjernst May 2, 2017

jasontedor commented May 3, 2017

rjernst commented May 9, 2017

dliappis commented May 9, 2017

rjernst commented May 11, 2017

jasontedor left a comment

		@@ -44,6 +47,9 @@

		public class InternalSettingsPreparer {

		private static Logger logger = ESLoggerFactory.getLogger("settings");

Deprecate settings in .yml and .json #24059

Deprecate settings in .yml and .json #24059

Conversation

rjernst commented Apr 12, 2017

rjernst commented Apr 12, 2017

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst commented Apr 26, 2017

rjernst commented Apr 27, 2017

rjernst commented May 2, 2017

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor commented May 3, 2017

rjernst commented May 9, 2017

dliappis commented May 9, 2017

rjernst commented May 11, 2017

jasontedor left a comment

Choose a reason for hiding this comment