Skip to content

Reduce deprecation logging severity for settings that are not removed in 8.0#79665

Merged
jakelandis merged 12 commits intoelastic:7.16from
jakelandis:lower_settings_deprecation_warnings
Oct 27, 2021
Merged

Reduce deprecation logging severity for settings that are not removed in 8.0#79665
jakelandis merged 12 commits intoelastic:7.16from
jakelandis:lower_settings_deprecation_warnings

Conversation

@jakelandis
Copy link
Copy Markdown
Contributor

@jakelandis jakelandis commented Oct 21, 2021

This commit lowers the severity for all settings from critical to warning
for all settings that are still present in 8.0.0.

releated #79107

===

Note - I have requested review from @tvernum and @jbaiera since these settings are mostly from areas for which they are familiar. Please add others for review if needed.
cc: @masseyke @rjernst @pgomulka

===
Note this settings were identified by introducing the following debug code in 8.0.0 and 7.16 and comparing the two lists. If the same setting showed up as deprecated in both 7.16 and 8.0.0 it is included in this PR. If the setting was missing in 8.0.0, no changes since the default is critical.

diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java
index a56baf6cf3b..8a23f9ce906 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java
@@ -161,6 +161,12 @@ public class Setting<T> implements ToXContentObject {
         this.defaultValue = defaultValue;
         this.parser = parser;
         this.validator = validator;
+        if(Arrays.stream(properties).filter(p -> p.equals(Property.Deprecated) || p.equals(Property.DeprecatedWarning)).count() > 0){
+                       System.out.println("**deprecated setting: " + key +
+                           (Arrays.stream(properties).filter( p -> p.equals(Property.DeprecatedWarning)).count() > 0 ? "(warning)" : ""));
+
+
+                    }
         if (properties == null) {
             throw new IllegalArgumentException("properties cannot be null for setting [" + key + "]");
         }

@jakelandis jakelandis added >non-issue :Core/Infra/Settings Settings infrastructure and APIs v7.16.0 labels Oct 21, 2021
@jakelandis jakelandis requested review from jbaiera and tvernum October 21, 2021 22:44
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Oct 21, 2021
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Oct 21, 2021

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

@jakelandis jakelandis changed the title Reduce deprecration logging severity for settings that are not removed in 8.0 Reduce deprecation logging severity for settings that are not removed in 8.0 Oct 21, 2021
@jakelandis jakelandis mentioned this pull request Oct 21, 2021
Closed
jbaiera
jbaiera approved these changes Oct 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

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

LGTM on the monitoring changes, though I think just one got missed

...ck/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java
public static final Setting.AffixSetting<Boolean> USE_INGEST_PIPELINE_SETTING =
Setting.affixKeySetting("xpack.monitoring.exporters.","use_ingest",
key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope, Property.Deprecated), TYPE_DEPENDENCY);
key -> Setting.boolSetting(key, true, Property.Dynamic, Property.NodeScope, Property.Deprecated),
Copy link
Copy Markdown
Member

@jbaiera jbaiera Oct 25, 2021

Choose a reason for hiding this comment

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

I think this one should be at warning level as well?

Copy link
Copy Markdown
Contributor Author

@jakelandis jakelandis Oct 26, 2021

Choose a reason for hiding this comment

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

That setting was removed in 8.x, so it should remain critical level .

tvernum
tvernum approved these changes Oct 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM.
In a few months we should have a conversation about what the deprecation lifecycle looks like and how setting attributes should work with that. But not now.

@danhermann danhermann added v8.1.0 and removed v7.16.0 labels Oct 27, 2021
@jakelandis
Copy link
Copy Markdown
Contributor Author

jakelandis commented Oct 27, 2021

@elasticmachine update branch

@jakelandis jakelandis merged commit 6e3a108 into elastic:7.16 Oct 27, 2021
@jakelandis jakelandis deleted the lower_settings_deprecation_warnings branch October 27, 2021 15:08
@jakelandis jakelandis mentioned this pull request Nov 4, 2021
Merged
jakelandis added a commit that referenced this pull request Nov 10, 2021
@jakelandis
[7.16] Drop non-setting deprecations down to warning level (#80374)
6358afb 
Complementary to #79665 this change
will reduce the deprecation logging level to WARN for (most) all non-settings that
continue to exist in 8.0+.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbaiera jbaiera jbaiera approved these changes

@tvernum tvernum tvernum approved these changes

Assignees

No one assigned

Labels

:Core/Infra/Settings Settings infrastructure and APIs >non-issue Team:Core/Infra Meta label for core/infra team v7.16.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jakelandis @elasticmachine @jbaiera @tvernum @mark-vieira @danhermann @elasticsearchmachine