Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
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
6 changes: 6 additions & 0 deletions docs/changelog/135886.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 135886
summary: Provide defaults for index sort settings
area: Mapping
type: bug
issues:
- 129062
3 changes: 3 additions & 0 deletions rest-api-spec/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ tasks.named("yamlRestCompatTestTransform").configure ({ task ->
task.skipTest("update/100_synthetic_source/stored text", "synthetic recovery source means _recovery_source field will not be present")
task.skipTest("logsdb/10_settings/start time not allowed in logs mode", "we don't validate for index_mode=tsdb when setting start_date/end_date anymore")
task.skipTest("logsdb/10_settings/end time not allowed in logs mode", "we don't validate for index_mode=tsdb when setting start_date/end_date anymore")
task.skipTest("logsdb/10_settings/override sort mode settings", "we changed the error message")
task.skipTest("logsdb/10_settings/override sort missing settings", "we changed the error message")
task.skipTest("logsdb/10_settings/override sort order settings", "we changed the error message")
task.skipTest("tsdb/10_settings/set start_time and end_time without timeseries mode", "we don't validate for index_mode=tsdb when setting start_date/end_date anymore")
task.skipTest("tsdb/10_settings/set start_time, end_time and routing_path via put settings api without time_series mode", "we don't validate for index_mode=tsdb when setting start_date/end_date anymore")
// Expected deprecation warning to compat yaml tests:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ override sort order settings:
type: text

- match: { error.type: "illegal_argument_exception" }
- match: { error.reason: "index.sort.fields:[] index.sort.order:[asc, asc], size mismatch" }
- match: { error.reason: "setting [index.sort.order] requires [index.sort.field] to be configured" }

---
override sort missing settings:
Expand Down Expand Up @@ -312,7 +312,7 @@ override sort missing settings:
type: text

- match: { error.type: "illegal_argument_exception" }
- match: { error.reason: "index.sort.fields:[] index.sort.missing:[_last, _first], size mismatch" }
- match: { error.reason: "setting [index.sort.missing] requires [index.sort.field] to be configured" }

---
override sort mode settings:
Expand Down Expand Up @@ -348,7 +348,7 @@ override sort mode settings:
type: text

- match: { error.type: "illegal_argument_exception" }
- match: { error.reason: "index.sort.fields:[] index.sort.mode:[MAX, MAX], size mismatch" }
- match: { error.reason: "setting [index.sort.mode] requires [index.sort.field] to be configured" }

---
override sort field using nested field type in sorting:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -576,3 +576,29 @@ check time_series empty time bound value:
"@timestamp": "2021-09-26T03:09:52.123456789Z",
"metricset": "pod"
}

---
default sort field:
- requires:
cluster_features: [ "mapper.provide_index_sort_setting_defaults" ]
reason: "testing index sort setting defaults"

- do:
indices.create:
index: test_index
body:
settings:
index:
mode: time_series
routing_path: foo
time_series:
start_time: 2021-04-28T00:00:00Z
end_time: 2021-04-29T00:00:00Z

- do:
indices.get_settings:
index: test_index
include_defaults: true
- match: { .test_index.settings.index.mode: time_series }
- match: { .test_index.defaults.index.sort.field: [ "_tsid", "@timestamp" ] }
- match: { .test_index.defaults.index.sort.order: [ "asc", "desc" ] }
Original file line number Diff line number Diff line change
Expand Up @@ -1821,11 +1821,19 @@ public static Setting<List<String>> stringListSetting(String key, Property... pr
}

public static Setting<List<String>> stringListSetting(String key, List<String> defValue, Property... properties) {
return new ListSetting<>(key, null, s -> defValue, s -> parseableStringToList(s, Function.identity()), v -> {}, properties) {
return stringListSettingWithDefaultProvider(key, s -> defValue, properties);
}

public static Setting<List<String>> stringListSettingWithDefaultProvider(
String key,
Function<Settings, List<String>> defValueProvider,
Property... properties
) {
return new ListSetting<>(key, null, defValueProvider, s -> parseableStringToList(s, Function.identity()), v -> {}, properties) {
@Override
public List<String> get(Settings settings) {
checkDeprecation(settings);
return settings.getAsList(getKey(), defValue);
return settings.getAsList(getKey(), defValueProvider.apply(settings));
}
};
}
Expand All @@ -1852,6 +1860,15 @@ public static <T> Setting<List<T>> listSetting(
return listSetting(key, null, singleValueParser, s -> defaultStringValue, properties);
}

public static <T> Setting<List<T>> listSetting(
final String key,
final Function<Settings, List<String>> defaultStringValueProvider,
final Function<String, T> singleValueParser,
final Property... properties
) {
return listSetting(key, null, singleValueParser, defaultStringValueProvider, properties);
}

public static <T> Setting<List<T>> listSetting(
final String key,
final List<String> defaultStringValue,
Expand Down Expand Up @@ -1981,7 +1998,7 @@ public void diff(Settings.Builder builder, Settings source, Settings defaultSett
if (exists(source) == false) {
List<String> asList = defaultSettings.getAsList(getKey(), null);
if (asList == null) {
builder.putList(getKey(), defaultStringValue.apply(defaultSettings));
builder.putList(getKey(), defaultStringValue.apply(source));
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 think this is a bug, but I'm not sure why we never saw it before. Maybe we never had string list settings whose default value depended on other settings before?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find, definitely looks like a bug to me

Copy link
Member

Choose a reason for hiding this comment

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

Good find, I agree that the current behaviour isn't correct and actual settings need to be pushed down instead of default settings..

} else {
builder.putList(getKey(), asList);
}
Expand Down
5 changes: 4 additions & 1 deletion server/src/main/java/org/elasticsearch/index/IndexMode.java
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,11 @@ void validateWithOtherSettings(Map<Setting<?>, Object> settings) {
if (settings.get(IndexMetadata.INDEX_ROUTING_PARTITION_SIZE_SETTING) != Integer.valueOf(1)) {
throw new IllegalArgumentException(error(IndexMetadata.INDEX_ROUTING_PARTITION_SIZE_SETTING));
}

var settingsWithIndexMode = Settings.builder().put(IndexSettings.MODE.getKey(), getName()).build();

for (Setting<?> unsupported : TIME_SERIES_UNSUPPORTED) {
if (false == Objects.equals(unsupported.getDefault(Settings.EMPTY), settings.get(unsupported))) {
if (false == Objects.equals(unsupported.getDefault(settingsWithIndexMode), settings.get(unsupported))) {
throw new IllegalArgumentException(error(unsupported));
}
}
Expand Down
Loading