Enable synthetic id feature, remove feature flag#142366
Enable synthetic id feature, remove feature flag#142366burqen wants to merge 48 commits intoelastic:mainfrom
Conversation
The feature flag `tsdb_synthetic_id` is removed together with all usages of it. This makes setting `index.mapping.synthetic_id`, which controls the usage of synthetic ids for time series indexes, accessible without any special system properties and thus the synthetic id feature is officially available. The default behaviour is still index.mapping.synthetic_id=false however and user must specifically set it to true on (time series) index or data stream creation.
There was a problem hiding this comment.
Pull request overview
This PR removes the tsdb_synthetic_id feature flag and makes index.mapping.synthetic_id generally configurable (still defaulting to false), updating server-side validation/metadata and adjusting tests to no longer depend on the flag.
Changes:
- Removed the
tsdb_synthetic_idfeature flag (and its propagation viaFeatureFlag). - Made
index.mapping.synthetic_ida built-in index setting (no longer conditionally registered) and removed flag-based gating in server logic. - Updated multiple test suites to stop assuming the flag and to optionally/always exercise synthetic-id behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| x-pack/plugin/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TimeSeriesIT.java | Stops gating synthetic-id usage behind the removed feature flag. |
| x-pack/plugin/esql/qa/server/single-node/src/javaRestTest/java/org/elasticsearch/xpack/esql/qa/single_node/TSDBRestEsqlIT.java | Removes feature-flag gating and randomly enables synthetic ids for the test index. |
| x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/CcrTimeSeriesDataStreamsIT.java | Removes feature-flag assumption so the synthetic-id CCR test always runs. |
| test/test-clusters/src/main/java/org/elasticsearch/test/cluster/FeatureFlag.java | Removes the TSDB_SYNTHETIC_ID_FEATURE_FLAG enum entry. |
| server/src/test/java/org/elasticsearch/index/codec/tsdb/TSDBSyntheticIdPostingsFormatTests.java | Removes feature-flag assumptions so tests always run. |
| server/src/test/java/org/elasticsearch/index/codec/CodecTests.java | Removes feature-flag assumptions and related assertions. |
| server/src/main/java/org/elasticsearch/index/IndexSettings.java | Removes feature-flag gating from SYNTHETIC_ID validation and enables synthetic-id usage based purely on the setting. |
| server/src/main/java/org/elasticsearch/common/settings/IndexScopedSettings.java | Registers IndexSettings.SYNTHETIC_ID unconditionally as a built-in index setting. |
| server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java | Removes feature-flag gating when deriving synthetic-id usage from settings/versions. |
| modules/data-streams/src/javaRestTest/java/org/elasticsearch/datastreams/TsdbDataStreamRestIT.java | Removes cluster feature-flag enabling block. |
| modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBSyntheticIdsIT.java | Removes feature-flag assumptions so the suite always runs. |
| modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBPassthroughIndexingIT.java | Removes feature-flag gating and randomly enables synthetic ids in templates. |
| modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBIndexingIT.java | Removes feature-flag gating and randomly enables synthetic ids in templates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public void testSyntheticId() throws Exception { | ||
| assumeTrue("Test should only run with feature flag", IndexSettings.TSDB_SYNTHETIC_ID_FEATURE_FLAG); | ||
| final var dataStreamName = randomIdentifier(); | ||
| putDataStreamTemplate(dataStreamName, randomIntBetween(1, 5), 0); | ||
|
|
There was a problem hiding this comment.
Now that the feature-flag assumes are removed, this suite will always create TSDB indices with index.mapping.synthetic_id=true. Because this class is annotated with @SuppressCodecs("*"), ESIntegTestCase may set the default index.codec to best_compression for the test cluster, which will violate SYNTHETIC_ID validation (requires index.codec=default) and make the tests fail/flaky. Consider forcing index.codec to "default" in the template/index settings used by this suite when synthetic ids are enabled.
| public void validate(Boolean enabled) {} | ||
|
|
||
| @Override |
There was a problem hiding this comment.
The SYNTHETIC_ID setting’s Validator now overrides validate(Boolean) with an empty implementation. Since the dependency-based validation is already implemented in validate(Boolean, Map), consider removing this no-op override entirely to avoid implying there is intentional standalone validation here.
| public void validate(Boolean enabled) {} | |
| @Override |
| ); | ||
|
|
||
| public static final boolean TSDB_SYNTHETIC_ID_FEATURE_FLAG = new FeatureFlag("tsdb_synthetic_id").isEnabled(); | ||
| public static final Setting<Boolean> SYNTHETIC_ID = Setting.boolSetting("index.mapping.synthetic_id", false, new Setting.Validator<>() { |
There was a problem hiding this comment.
I think that we should enable it by default? @burqen
…etic-id-remove-feature-flag
…etic-id-remove-feature-flag
`index.mapping.synthetic_id` defaults to true if - index mode is time_series - index version is TIME_SERIES_USE_SYNTHETIC_ID_94 or later - codec is default or unset This default logic is duplicated by IndexMetadata. MODE setting moved above SYNTHETIC_ID to allow access.
... in TSDBSyntheticIdsIT. Behaviour should be the same since default is now true
- Add block comment for TSDB synthetic ID / feature flag removal - Mute 13 YAML tests across ClientYamlTestSuiteIT, SmokeTestMultiNode, and CoreWithSecurityClientYamlTestSuiteIT - Mute TsdbIT.testTsdbDataStream and RandomizedTimeSeriesIT.testGaugeGroupByRandomAndRandomAgg - Order methods alphabetically by yaml path for consistency Co-authored-by: Cursor <cursoragent@cursor.com>
muted-tests.yml
Outdated
| - class: org.elasticsearch.xpack.esql.qa.multi_node.GenerativeIT | ||
| method: test | ||
| issue: https://github.com/elastic/elasticsearch/issues/142426 | ||
| # TSDB synthetic ID / feature flag removal - PR 142366 |
There was a problem hiding this comment.
ClientYamlTestSuiteIT, SmokeTestMultiNodeClientYamlTestSuiteIT and CoreWithSecurityClientYamlTestSuiteIT all fail on the same tests. Here is a more digestible summary
- class: org.elasticsearch.test.rest.ClientYamlTestSuiteIT
- class: org.elasticsearch.smoketest.SmokeTestMultiNodeClientYamlTestSuiteIT
- class: org.elasticsearch.xpack.security.CoreWithSecurityClientYamlTestSuiteIT
method: test {yaml=delete/70_tsdb/basic tsdb delete}
method: test {yaml=tsdb/160_nested_fields/Create TSDB index with field of nested type}
method: test {yaml=tsdb/160_nested_fields/TSDB index with multi-level nested fields}
method: test {yaml=tsdb/25_id_generation/create operation on top of old document fails}
method: test {yaml=tsdb/25_id_generation/create operation on top of old document fails over bulk}
method: test {yaml=tsdb/25_id_generation/delete over _bulk}
method: test {yaml=tsdb/25_id_generation/generates a consistent id}
method: test {yaml=tsdb/25_id_generation/get}
method: test {yaml=tsdb/25_id_generation/ids query}
method: test {yaml=tsdb/25_id_generation/index a new document on top of an old one}
method: test {yaml=tsdb/25_id_generation/index a new document on top of an old one over bulk}
method: test {yaml=tsdb/25_id_generation/routing_path matches deep object}
method: test {yaml=tsdb/25_id_generation/routing_path matches object}
- class: org.elasticsearch.xpack.logsdb.TsdbIT
method: testTsdbDataStream
# Failing seed: 2F7CB88BCB870938. Assertion in VersionsAndSeqNoResolver.timeSeriesLoadDocIdAndVersion (line 174)
- class: org.elasticsearch.xpack.esql.action.RandomizedTimeSeriesIT
method: testGaugeGroupByRandomAndRandomAgg
There was a problem hiding this comment.
Thanks for the summary, it seems that the issue is mostly about explicit _id's in the tests. I wonder if we could rely on the cluster_features filter to modify the tests and skip it in previous versions? I'm not 100% sure if these test suites run in BwC mode.
cluster_features: "gte_v8.14.0"
muted-tests.yml
Outdated
| - class: org.elasticsearch.xpack.security.CoreWithSecurityClientYamlTestSuiteIT | ||
| method: test {yaml=tsdb/25_id_generation/routing_path matches object} | ||
| issue: https://github.com/elastic/elasticsearch/pull/142366 | ||
| - class: org.elasticsearch.multiproject.test.CoreWithMultipleProjectsClientYamlTestSuiteIT |
There was a problem hiding this comment.
Another batch of muted test. The yaml tests are the same, the others come in three flavors:
### Routing hash problems
- SourceFieldMapperTests
- IndexRoutingTests
- TextFieldMapperTests
- BinaryFieldMapperTests
Example SourceFieldMapperTests.testRecoverySourceWithTimeSeriesCustom
Reproduce:
./gradlew ":server:test" --tests "org.elasticsearch.index.mapper.SourceFieldMapperTests.testRecoverySourceWithTimeSeriesCustom" -Dtests.seed=61EBD4FBE6039DEA -Dtests.locale=ca-Latn-ES -Dtests.timezone=Europe/Zaporozhye -Druntime.java=25
Cause:
java.lang.AssertionError at org.elasticsearch.index.mapper.TsidExtractingIdFieldMapper.extractRoutingHashFromSyntheticId(TsidExtractingIdFieldMapper.java:202) (via TimeSeriesRoutingHashFieldMapper.postParse → DocumentParser → SourceFieldMapperTests.testRecoverySourceWithTimeSeriesCustom(SourceFieldMapperTests.java:786)).
### Class instance failure
- PerFieldMapperCodecTests
example: testUseEs812PostingsFormatForIdField
Reproduce:
./gradlew ":server:test" --tests "org.elasticsearch.index.codec.PerFieldMapperCodecTests.testUseEs812PostingsFormatForIdField" -Dtests.seed=61EBD4FBE6039DEA -Dtests.locale=ia-Latn-001 -Dtests.timezone=Asia/Ulaanbaatar -Druntime.java=25
Cause:
PerFieldMapperCodecTests > testUseEs812PostingsFormatForIdField FAILED
java.lang.AssertionError:
Expected: an instance of org.elasticsearch.index.codec.postings.ES812PostingsFormat
but: <PostingsFormat(name=TSDBSyntheticId)> is a org.elasticsearch.index.codec.tsdb.TSDBSyntheticIdPostingsFormat
### Wrong id (hard coded)
- TsidExtractingIdFieldMapperTests
Reproduce (entire class):
./gradlew ":server:test" --tests "org.elasticsearch.index.mapper.TsidExtractingIdFieldMapperTests" -Dtests.seed=61EBD4FBE6039DEA -Dtests.locale=mt-Latn-MT -Dtests.timezone=Eire -Druntime.java=25
Example single-test reproduce:
./gradlew ":server:test" --tests "org.elasticsearch.index.mapper.TsidExtractingIdFieldMapperTests.testProvideExpectedIdWithRoutingPath {p0=2022-01-01T01:00:00Z}" -Dtests.seed=61EBD4FBE6039DEA -Dtests.locale=mt-Latn-MT -Dtests.timezone=Eire -Druntime.java=25
Cause:
org.elasticsearch.index.mapper.DocumentParsingException: [4:1] failed to parse: \_id must be unset or set to [JJSLNivCxv3hDTQtWd6qGUwGlT_5e6_NYGOZWULpmMG9IAlZlH___oHs2XV_AAAABw] but was [BwAAAKjcFfi45iV3AAABfhMmioA] because [index] is in time_series mode
(and similarly for other parameterized cases, e.g. [-1:40] ... but was [BwAAAEk383E-IPhiAAABfhMmioA] because [index] is in time_series mode).
Some failures show as AssertionError: Expected: "<expected_id>" (ID format / TSID expectation mismatch).
Clump tests together by cause of failure for better overview on what to unmute when issues are fixed. Also remove mute of TsdbIT.testTsdbDataStream because it fails for unrelated reasons.
…etic-id-remove-feature-flag
✅ Vale Linting ResultsNo issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
🔍 Preview links for changed docs |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
…etic-id-remove-feature-flag
26d8dde to
21452b2
Compare
…etic-id-remove-feature-flag
…etic-id-remove-feature-flag
Since we hide synthetic id setting behind new IndexVersion, we also need to introduce a new NodeFeature in order for tests to behave correctly. Without updating the NodeFeature, tests would expect the feature to work in cluster where it will not and instead cause broken cluster states.
Change order of minVersion if check because IndexVersions.TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT is greater than IndexVersions.DISABLE_SEQUENCE_NUMBERS
…etic-id-remove-feature-flag
- IndexRoutingTests - TSDBIndexingIT
When the TSDB synthetic ID feature flag is enabled, disable synthetic _id in recovery source tests since the hardcoded "123" id is not synthetic-id-conformant and synthetic ids are not relevant to these tests. Relates elastic#142366
When the TSDB synthetic ID feature flag is enabled, disable synthetic _id in recovery source tests since the hardcoded "123" id is not synthetic-id-conformant and synthetic ids are not relevant to these tests. Relates #142366
index.time_series_synthetic_id was previously replaced by index.time_series_synthetic_id_default, but this was a mistake. The cluster cannot form during upgrade if new nodes don't have the NodeFeatures supported by the old nodes. So instead of removing the old feature and replacing with the new, we let both live side by side in new nodes.
…etic-id-remove-feature-flag
…etic-id-remove-feature-flag
|
This draft has been successful in weeding out the failing tests and it's now time for it to retire to leave way for the new kids on the block: |
The feature flag
tsdb_synthetic_idis removed together with all usages of it. This makes settingindex.mapping.synthetic_id, which controls the usage of synthetic ids for time series indexes, accessible without any special system properties and thus the synthetic id feature is officially available. The default behaviour is still index.mapping.synthetic_id=false however and user must specifically set it to true on (time series) index or data stream creation.Known test failures
TsidExtractingIdFieldMapper.extractRoutingHashFromSyntheticId"The rolling upgrade problem"
We are doing a rolling upgrade from 9.4-pre to 9.4-post.
TIME_SERIES_USE_SYNTHETIC_ID_94but does not use synthetic id by default for time_series indices.Behavior change in existing indices
In serverless we do rolling upgrade from whatever is the latest promoted version (let's call it 9.4-pre) to new promoted version (9.4-post). A time_series index created in 9.4-pre will not use synthetic id by default (and will not have the synthetic_id setting, but it will have an indexVersion that allows synthetic id to be used (
IndexVersions.TIME_SERIES_USE_SYNTHETIC_ID_94).After upgrading to 9.4-post where synthetic ids are used by default for time_series this existing index will change default behavior to using synthetic ids. This is bad! (Causes failures like: "cannot change field "_id" from doc values type=NONE to inconsistent doc values type=BINARY").
SOLUTION!
To make sure index default behavior stay consistent before and after enabling synthetic id by default we introduce a new IndexVersion (
IndexVersions.TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT). Indices before this version have OFF as default, indices onOrAfter this version have ON as default.Indices that are not forward compatible
This is great, but it introduces another problem. In 9.4-pre it is perfectly valid to create an index with synthetic_id=true. From the point of view of 9.4-pre, it has the right IndexVersion (
TIME_SERIES_USE_SYNTHETIC_ID_94). But when we upgrade to 9.4-post,TIME_SERIES_USE_SYNTHETIC_ID_94is no longer enough to acceptindex.mapping.synthetic_id,TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT. So the index created in 9.4-pre or during the rolling upgrade is now considered a broken cluster state and this will prevent the new cluster from forming."But synthetic id should be hidden behind the feature flag in 9.4-pre so it should not be possible to create a synthetic id index anyway." Yes, that's correct in production. But we also run tests for -snapshot versions and those have the feature flag enabled.
SOLUTION!
Introduce a new setting that creates a clean cutoff point between 9.4-pre and 9.4-post in the same commit that introduce the new index version. This is the same thing that we did between 9.3/9.4 #141525.
Index feature helper is broken
The index / node feature
index.time_series_synthetic_idis a shortcut for checking if all nodes in the cluster has support for synthetic id (has support for a high enough index version). It essentially means if all nodes haveindex.time_series_synthetic_id, then they also have index versionTIME_SERIES_USE_SYNTHETIC_ID_94. With the new index version, this is not enough. We need a new index/node feature forTIME_SERIES_USE_SYNTHETIC_ID_DEFAULT. We could also skip the node feature entirely and check index version directly, but some test infrastructure like the yaml tests rely on node feature and will probably be difficult to replace. Example fromrest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/160_nested_fields.yml:SOLUTION!
Introduce a new index / node feature in the same commit that introduce the index version.