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

Adding a deprecation info API check for fractional byte value settings #77074

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ private DeprecationChecks() {
NodeDeprecationChecks::checkSingleDataNodeWatermarkSetting,
NodeDeprecationChecks::checkImplicitlyDisabledSecurityOnBasicAndTrial,
NodeDeprecationChecks::checkMonitoringExporterPassword,
NodeDeprecationChecks::checkFractionalByteValueSettings,
NodeDeprecationChecks::checkClusterRoutingAllocationIncludeRelocationsSetting
)
).collect(Collectors.toList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.util.concurrent.EsExecutors;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.env.Environment;
Expand All @@ -38,6 +39,7 @@

import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -595,4 +597,31 @@ static DeprecationIssue checkClusterRoutingAllocationIncludeRelocationsSetting(f
DeprecationIssue.Level.CRITICAL
);
}

static DeprecationIssue checkFractionalByteValueSettings(final Settings settings,
final PluginsAndModules pluginsAndModules,
final ClusterState clusterState,
final XPackLicenseState licenseState) {
Map<String, String> fractionalByteSettings = new HashMap<>();
for (String key : settings.keySet()) {
try {
settings.getAsBytesSize(key, ByteSizeValue.ZERO);
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it triggers deprecation logging as well for each fractional value read, do we want that?

String stringValue = settings.get(key);
if (stringValue.contains(".")) {
fractionalByteSettings.put(key, stringValue);
}
} catch (Exception ignoreThis) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm not a huge fan of Exception based control flow. I know that settings are registered with ClusterSettings and IndexScopedSettings so that settings can be validated against the master set, but I don't know if we can filter on the setting's data type due to type erasure, and I'm not sure if those are comprehensive for every setting either.

// We expect anything that is not a byte setting to throw an exception, but we don't care about those
}
}
if (fractionalByteSettings.isEmpty()) {
return null;
}
String url = "https://www.elastic.co/guide/en/elasticsearch/reference/master/logging.html#deprecation-logging";
String message = "support for fractional byte size values is deprecated and will be removed in a future release";
String details = "change the following settings to non-fractional values: [" +
fractionalByteSettings.entrySet().stream().map(fractionalByteSetting -> fractionalByteSetting.getKey() + "->" +
fractionalByteSetting.getValue()).collect(Collectors.joining(", ")) + "]";
return new DeprecationIssue(DeprecationIssue.Level.WARNING, message, url, details, false, null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -863,4 +863,32 @@ public void testImplicitlyConfiguredSecurityOnGoldPlus() {
final List<DeprecationIssue> issues = getDeprecationIssues(settings, pluginsAndModules, licenseState);
assertThat(issues, empty());
}

public void testCheckFractionalByteValueSettings() {
String settingKey = "network.tcp.send_buffer_size";
String unit = randomFrom(new String[] {"k", "kb", "m", "mb", "g", "gb", "t", "tb", "p", "pb"});
float value = Math.abs(randomFloat());
String settingValue = value + unit;
String unaffectedSettingKey = "some.other.setting";
String unaffectedSettingValue = "54.32.43mb"; //Not an actual number, so we don't expect to see a deprecation log about it
final Settings nodeSettings =
Settings.builder().put(settingKey, settingValue).put(unaffectedSettingKey, unaffectedSettingValue).build();
final XPackLicenseState licenseState = new XPackLicenseState(Settings.EMPTY, () -> 0);
final ClusterState clusterState = ClusterState.EMPTY_STATE;
final DeprecationIssue expectedIssue = new DeprecationIssue(DeprecationIssue.Level.WARNING,
"support for fractional byte size values is deprecated and will be removed in a future release",
"https://www.elastic.co/guide/en/elasticsearch/reference/master/logging.html#deprecation-logging",
String.format(Locale.ROOT,
"change the following settings to non-fractional values: [%s->%s]",
settingKey,
settingValue),
false, null
);
assertThat(
NodeDeprecationChecks.checkFractionalByteValueSettings(nodeSettings, null, clusterState, licenseState),
equalTo(expectedIssue)
);
assertWarnings(String.format(Locale.ROOT,"Fractional bytes values are deprecated. Use non-fractional bytes values instead: [%s] " +
"found for setting [%s]", settingValue, settingKey));
}
}