Skip to content

Commit

Permalink
[8.x] Fix synthetic recovery source version validation. (#118942)
Browse files Browse the repository at this point in the history
* Fix synthetic recovery source version validation. (#118924)

The validation didn't work as expected, because created version was always 0 (which is created version default value).

Moving the validation to a place that does have access to the index version.

* fixed compile error after cherry picking

* iter
  • Loading branch information
martijnvg authored Dec 18, 2024
1 parent 19fd296 commit f1eac4c
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 41 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2012,3 +2012,43 @@ synthetic_source with copy_to pointing inside dynamic object:
hits.hits.2.fields:
c.copy.keyword: [ "hello", "zap" ]

---
create index with use_synthetic_source:
- requires:
cluster_features: ["mapper.synthetic_recovery_source"]
reason: requires synthetic recovery source

- do:
indices.create:
index: test
body:
settings:
index:
recovery:
use_synthetic_source: true
mapping:
source:
mode: synthetic

- do:
indices.get_settings: {}
- match: { test.settings.index.mapping.source.mode: synthetic}
- is_true: test.settings.index.recovery.use_synthetic_source

- do:
bulk:
index: test
refresh: true
body:
- '{ "create": { } }'
- '{ "field": "aaaa" }'
- '{ "create": { } }'
- '{ "field": "bbbb" }'

- do:
indices.disk_usage:
index: test
run_expensive_tasks: true
flush: false
- gt: { test.store_size_in_bytes: 0 }
- is_false: test.fields._recovery_source
30 changes: 15 additions & 15 deletions server/src/main/java/org/elasticsearch/index/IndexSettings.java
Original file line number Diff line number Diff line change
Expand Up @@ -688,25 +688,11 @@ public void validate(Boolean enabled, Map<Setting<?>, Object> settings) {
);
}
}

// Verify that all nodes can handle this setting
var version = (IndexVersion) settings.get(SETTING_INDEX_VERSION_CREATED);
if (version.before(IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY_BACKPORT)) {
throw new IllegalArgumentException(
String.format(
Locale.ROOT,
"The setting [%s] is unavailable on this cluster because some nodes are running older "
+ "versions that do not support it. Please upgrade all nodes to the latest version "
+ "and try again.",
RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey()
)
);
}
}

@Override
public Iterator<Setting<?>> settings() {
List<Setting<?>> res = List.of(INDEX_MAPPER_SOURCE_MODE_SETTING, SETTING_INDEX_VERSION_CREATED, MODE);
List<Setting<?>> res = List.of(INDEX_MAPPER_SOURCE_MODE_SETTING, MODE);
return res.iterator();
}
},
Expand Down Expand Up @@ -1049,6 +1035,20 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti
indexMappingSourceMode = scopedSettings.get(INDEX_MAPPER_SOURCE_MODE_SETTING);
recoverySourceEnabled = RecoverySettings.INDICES_RECOVERY_SOURCE_ENABLED_SETTING.get(nodeSettings);
recoverySourceSyntheticEnabled = scopedSettings.get(RECOVERY_USE_SYNTHETIC_SOURCE_SETTING);
if (recoverySourceSyntheticEnabled) {
// Verify that all nodes can handle this setting
if (version.before(IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY_BACKPORT)) {
throw new IllegalArgumentException(
String.format(
Locale.ROOT,
"The setting [%s] is unavailable on this cluster because some nodes are running older "
+ "versions that do not support it. Please upgrade all nodes to the latest version "
+ "and try again.",
RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey()
)
);
}
}

scopedSettings.addSettingsUpdateConsumer(
MergePolicyConfig.INDEX_COMPOUND_FORMAT_SETTING,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ public Set<NodeFeature> getTestFeatures() {
DocumentParser.FIX_PARSING_SUBOBJECTS_FALSE_DYNAMIC_FALSE,
CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX,
META_FETCH_FIELDS_ERROR_CODE_CHANGED,
SPARSE_VECTOR_STORE_SUPPORT
SPARSE_VECTOR_STORE_SUPPORT,
SourceFieldMapper.SYNTHETIC_RECOVERY_SOURCE
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class SourceFieldMapper extends MetadataFieldMapper {
"mapper.source.remove_synthetic_source_only_validation"
);
public static final NodeFeature SOURCE_MODE_FROM_INDEX_SETTING = new NodeFeature("mapper.source.mode_from_index_setting");
public static final NodeFeature SYNTHETIC_RECOVERY_SOURCE = new NodeFeature("mapper.synthetic_recovery_source");

public static final String NAME = "_source";
public static final String RECOVERY_SOURCE_NAME = "_recovery_source";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -465,31 +465,6 @@ public void testRecoverySourceWitInvalidSettings() {
)
);
}
{
Settings settings = Settings.builder()
.put(SourceFieldMapper.INDEX_MAPPER_SOURCE_MODE_SETTING.getKey(), SourceFieldMapper.Mode.SYNTHETIC.toString())
.put(IndexSettings.RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey(), true)
.build();
IllegalArgumentException exc = expectThrows(
IllegalArgumentException.class,
() -> createMapperService(
IndexVersionUtils.randomPreviousCompatibleVersion(random(), IndexVersions.USE_SYNTHETIC_SOURCE_FOR_RECOVERY_BACKPORT),
settings,
() -> false,
topMapping(b -> {})
)
);
assertThat(
exc.getMessage(),
containsString(
String.format(
Locale.ROOT,
"The setting [%s] is unavailable on this cluster",
IndexSettings.RECOVERY_USE_SYNTHETIC_SOURCE_SETTING.getKey()
)
)
);
}
}

public void testRecoverySourceWithSyntheticSource() throws IOException {
Expand Down

0 comments on commit f1eac4c

Please sign in to comment.