Re-add support for some metadata field parameters #118825

cbuescher · 2024-12-17T09:40:06Z

We removed support for type, fields, copy_to and boost in metadata field definitions with #116944 but with the move towards supporting N-2 read-only indices we need to add them back. This change reverts previous removal commits and adapts tests to also check we now throw errors for newly created indices.

We removed support for type, fields, copy_to and boost in metadata field definitions with elastic#116944 but with the move towards supporting N-2 read-only indices we need to add them back. This change reverts previous removal commits and adapts tests to also check we now throw errors for newly created indices.

elasticsearchmachine · 2024-12-17T09:40:31Z

Pinging @elastic/es-search-foundations (Team:Search Foundations)

elasticsearchmachine · 2024-12-17T09:41:04Z

Hi @cbuescher, I've created a changelog YAML for you. Note that since this PR is labelled >breaking, you need to update the changelog YAML to fill out the extended information sections.

cbuescher · 2024-12-17T09:42:58Z

test/framework/src/main/java/org/elasticsearch/index/KnownIndexVersions.java

@@ -19,4 +19,6 @@ public class KnownIndexVersions {
     * A sorted list of all known transport versions
     */
    public static final List<IndexVersion> ALL_VERSIONS = List.copyOf(IndexVersions.getAllVersions());
+
+    public static final List<IndexVersion> ALL_READ_ONLY_VERSIONS = List.copyOf(IndexVersions.getAllReadOnlyVersions());


I'm not entirely sure why we need this list copy but since thats the pattern for ALL_VERSIONS I thought its save to do the same. Maybe someone know why we need that for ALL_VERSIONS?

By checking its history a bit, I think, the initial intention was to make a defensive copy of the list, which is immutable in the current version. The list was also unmodifiable when it was first introduced: #93384

I wonder if we need to have a list with read only versions. Could we instead just add a new method to get read only versions? Also, should we instead have a new method that returns all versions, writeable and read-only, as opposed to having special tests for read-only indices? My expectations it that the majority of the cases will need read support, and no write, hence the special case should probably be for writing and not for reading.

I think what I said above is necessarily correct. Switching randomizations to include read-only version cause quite a bit of issues. It makes sense to add for now a new list, we'll clean it up later. A question remains if that list should contain all versions including readonly or only read-only versions.

cbuescher · 2024-12-17T09:49:27Z

server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java

@@ -144,6 +150,22 @@ public final void parseMetadataField(String name, MappingParserContext parserCon
                final Object propNode = entry.getValue();
                Parameter<?> parameter = paramsMap.get(propName);
                if (parameter == null) {
+                    IndexVersion indexVersionCreated = parserContext.indexVersionCreated();
+                    if (indexVersionCreated.before(IndexVersions.UPGRADE_TO_LUCENE_10_0_0)


I'm not entirely sure if UPGRADE_TO_LUCENE_10_0_0 should be the point in time we should start to throw errors. We first removed support for these parameters with #116944 (8a7491c) at which point we already have more recent IndexVersions (see

elasticsearch/server/src/main/java/org/elasticsearch/index/IndexVersions.java

Line 135 in 8a7491c

public static final IndexVersion LOGSDB_DEFAULT_IGNORE_DYNAMIC_BEYOND_LIMIT = def(9_001_00_0, Version.LUCENE_10_0_0);

). To me that means indices with e.g. IndexVersion TIME_BASED_K_ORDERED_DOC_ID could still use those parameters? Please keep me honest here.

good point. Tricky question. I don't expect it to matter a lot because these unsupported params should not be common at all. The removal affects only serverless users indeed, but I'm afraid it can't be associated to a specific index version. If we meant to do so we'd have needed to add an index versions associated with the removal?

I am fine with using what you are using, or in alternative adding a new index version now.

javanna · 2024-12-17T11:17:27Z

server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java

@@ -144,6 +150,22 @@ public final void parseMetadataField(String name, MappingParserContext parserCon
                final Object propNode = entry.getValue();
                Parameter<?> parameter = paramsMap.get(propName);
                if (parameter == null) {
+                    IndexVersion indexVersionCreated = parserContext.indexVersionCreated();
+                    if (indexVersionCreated.before(IndexVersions.UPGRADE_TO_LUCENE_10_0_0)


good point. Tricky question. I don't expect it to matter a lot because these unsupported params should not be common at all. The removal affects only serverless users indeed, but I'm afraid it can't be associated to a specific index version. If we meant to do so we'd have needed to add an index versions associated with the removal?

I am fine with using what you are using, or in alternative adding a new index version now.

javanna · 2024-12-17T11:17:59Z

server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java

+                    IndexVersion indexVersionCreated = parserContext.indexVersionCreated();
+                    if (indexVersionCreated.before(IndexVersions.UPGRADE_TO_LUCENE_10_0_0)
+                        && UNSUPPORTED_PARAMETERS_8_6_0.contains(propName)) {
+                        if (indexVersionCreated.onOrAfter(IndexVersions.V_8_6_0)) {


I wonder if we should leave the deprecation under that conditional. What do you think?

I'm not sure I understand entirely. Under which conditions do you think we should issue a deprecation? As I see this atm before 8.6 we silently ignore, from 8.6 to 9 (or rather IndexVersions.UPGRADE_TO_LUCENE_10_0_0) we issue a deprecation and afterwards we throw. Do you mean reorganizing code to reflect that better or different behaviour?

I think I misread, we are good then.

javanna · 2024-12-17T11:20:21Z

test/framework/src/main/java/org/elasticsearch/index/KnownIndexVersions.java

@@ -19,4 +19,6 @@ public class KnownIndexVersions {
     * A sorted list of all known transport versions
     */
    public static final List<IndexVersion> ALL_VERSIONS = List.copyOf(IndexVersions.getAllVersions());
+
+    public static final List<IndexVersion> ALL_READ_ONLY_VERSIONS = List.copyOf(IndexVersions.getAllReadOnlyVersions());


I wonder if we need to have a list with read only versions. Could we instead just add a new method to get read only versions? Also, should we instead have a new method that returns all versions, writeable and read-only, as opposed to having special tests for read-only indices? My expectations it that the majority of the cases will need read support, and no write, hence the special case should probably be for writing and not for reading.

cbuescher · 2024-12-18T11:39:13Z

I'll wait for #118793 to get in before cleaning up and updating this PR

cbuescher · 2024-12-18T15:21:41Z

@javanna updates this with the index version randomization changes and left a small question for you.

cbuescher · 2024-12-18T20:23:17Z

@elasticmachine run elasticsearch-ci/part-1

cbuescher added :Search Foundations/Search Catch all for Search Foundations v9.0.0 labels Dec 17, 2024

cbuescher requested review from javanna and drempapis December 17, 2024 09:40

elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Dec 17, 2024

cbuescher added >breaking and removed Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Dec 17, 2024

elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Dec 17, 2024

Update docs/changelog/118825.yaml

e898670

cbuescher added :Search Foundations/Mapping Index mappings, including merging and defining field types and removed Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations labels Dec 17, 2024

elasticsearchmachine added the Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch label Dec 17, 2024

cbuescher commented Dec 17, 2024

View reviewed changes

iter

e8541bc

cbuescher commented Dec 17, 2024

View reviewed changes

javanna reviewed Dec 17, 2024

View reviewed changes

cbuescher added 2 commits December 18, 2024 16:13

Merge branch 'main' into revert-116944

2f81e13

iter

3048972

iter

ab3a39b

javanna approved these changes Dec 18, 2024

View reviewed changes

cbuescher merged commit cc69e06 into elastic:main Dec 18, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-add support for some metadata field parameters #118825

Re-add support for some metadata field parameters #118825

cbuescher commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

cbuescher Dec 17, 2024

drempapis Dec 17, 2024

javanna Dec 17, 2024

javanna Dec 17, 2024

cbuescher Dec 17, 2024

javanna Dec 17, 2024

javanna Dec 17, 2024

javanna Dec 17, 2024

cbuescher Dec 18, 2024

javanna Dec 18, 2024

javanna Dec 17, 2024

cbuescher commented Dec 18, 2024

cbuescher commented Dec 18, 2024

cbuescher commented Dec 18, 2024

Re-add support for some metadata field parameters #118825

Re-add support for some metadata field parameters #118825

Conversation

cbuescher commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

elasticsearchmachine commented Dec 17, 2024

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

cbuescher commented Dec 18, 2024

cbuescher commented Dec 18, 2024

cbuescher commented Dec 18, 2024