Enable synthetic_id by default under feature flag#144026
Enable synthetic_id by default under feature flag#144026burqen merged 24 commits intoelastic:mainfrom
Conversation
Newly created time_series indices use synthetic id by default. The new IndexVersion, TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT, prevent old indices (with older index version) from changing default behavior. `index.mapping.synthetic_id` defaults to true if - Feature flag TSDB_SYNTHETIC_ID_FEATURE_FLAG is enabled - index mode is time_series - index version is TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT or later - codec is default or unset and setting is still allowed to be explicitly set if index version is TIME_SERIES_USE_SYNTHETIC_ID_94 or later. Only the default behavior is controlled by TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT. This default logic is duplicated by IndexMetadata to make sure metadata always agree with Settings. MODE setting moved above SYNTHETIC_ID to allow access.
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| /** | ||
| * The {@link IndexMode "mode"} of the index. | ||
| */ | ||
| public static final Setting<IndexMode> MODE = Setting.enumSetting( |
There was a problem hiding this comment.
Needed to move this to have access in SYNTHETIC_ID, but the code is unchanged
fcofdez
left a comment
There was a problem hiding this comment.
Looks good! I think that you're right regarding the node feature, but maybe for safety we might want to bump that too?
...a-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/TSDBSyntheticIdsIT.java
Outdated
Show resolved
Hide resolved
Yeah, it will be difficult to introduce it retrospectively. We can't remove the existing one (it will prevent clusters from forming), but I'll add a new one in this PR. |
…etic-id-enable-by-default-behind-ff
Making sure to add this node feature in the same PR as adding the new index version, `TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT` to make sure the node feature can be used as a shortcut / replacement for checking version in tests.
The compat tests run with a copy of the yaml file from the BWC branch,
which doesn't contain the skip for index.time_series_synthetic_id.
This means the hardcoded ids in the test doesn't match with actual
default behavior anymore, so we skip those tests in the compatibility
run.
This is safe because the behavior is not changed for existing indices
that don't use synthetic id. Only new indices will have the changed
behavior, so there is no compatibility issue.
To make this work we nedd to make it possible to skip compatibility
tests with overloaded names.
If the name of the test is also a name of an operation such as `get` or
`delete` in below examples, then the build would fail with
```
Execution failed for task ':rest-api-spec:yamlRestCompatTestTransform'.
> class com.fasterxml.jackson.databind.node.ObjectNode cannot be cast to
class com.fasterxml.jackson.databind.node.ArrayNode
```
Instead of throwing an (assert) exception if the node to transform
(skip) is of the wrong type, we now just ignore it instead.
Example of test that could not be skipped before, but that can be
skipped now:
```
get:
- do:
get:
index: id_generation_test
id: cZZNs7B9sSWsyrL5AAABeRnSA5M
```
Written together with Cursor
This is an attempt at avoiding this failure: ``` TSDBSyntheticIdsIT > testDefaultSetting FAILED java.lang.AssertionError: searcher was not warmed up yet for source[assert_no_id_stored_field] ``` If replica hasn't caught up to primary before calling flushAndRefresh the replica would not be refreshed and isWarmedUp would not be set in InternalEngine.ExternalReaderManager.refreshIfNeeded. By calling ensureGreen we make sure that replicate has caught up and that searcher will be warmed up.
|
@elasticmachine update branch |
|
Another green run ✅ Triggering a third one |
| "Malformed values are now stored in binary doc values which sort differently than stored fields" | ||
| ) | ||
| task.skipTest("delete/70_tsdb/basic tsdb delete", "ids have changed after introduction of synthetic id") | ||
| task.skipTest("tsdb/25_id_generation/routing_path matches object", "ids have changed after introduction of synthetic id") |
There was a problem hiding this comment.
can't we set the setting to false in this test suite?
There was a problem hiding this comment.
This is only for the compatibility tests. They copy the old yaml file from the bwc branch and updating all of those is not worth it IMO.
|
Upgrade test failing to build buildBwcLinuxTar. Unrelated, retriggering the failing upgrade build step. |
|
Hi @mosche 👋 I see your name in the git blame of |
|
@elastic/es-delivery is probably more knowledgable on the these build tools |
|
The test failures from the last run doesn't look relevant, but will trigger a new one.
|
jozala
left a comment
There was a problem hiding this comment.
I've reviewed the change in the Skip.java file and it looks like a good improvement.
I've left one minor comment.
...-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/skip/Skip.java
Outdated
Show resolved
Hide resolved
…etic-id-enable-by-default-behind-ff
|
Last test failures:
|
…etic-id-enable-by-default-behind-ff
|
Latest test failures are a lot around ESQL: Trigger another build. |
…etic-id-enable-by-default-behind-ff
…etic-id-enable-by-default-behind-ff
|
Latest test failures
As theses two failures are known issues and unrelated to the enabling of |
* Enable synthetic_id by default if settings are valid
Newly created time_series indices use synthetic id by default.
The new IndexVersion, TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT, prevent old indices
(with older index version) from changing default behavior.
`index.mapping.synthetic_id` defaults to true if
- Feature flag TSDB_SYNTHETIC_ID_FEATURE_FLAG is enabled
- index mode is time_series
- index version is TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT or later
- codec is default or unset
and setting is still allowed to be explicitly set if index version
is TIME_SERIES_USE_SYNTHETIC_ID_94 or later. Only the default behavior
is controlled by TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT.
This default logic is duplicated by IndexMetadata to make sure metadata always
agree with Settings.
MODE setting moved above SYNTHETIC_ID to allow access.
* NodeFeature index.time_series_synthetic_id_default
Making sure to add this node feature in the same PR as adding the new
index version, `TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT` to make sure the
node feature can be used as a shortcut / replacement for checking
version in tests.
* Skip tsdb/25_id... and delete/70_tsdb in yamlRestCompatTest
The compat tests run with a copy of the yaml file from the BWC branch,
which doesn't contain the skip for index.time_series_synthetic_id.
This means the hardcoded ids in the test doesn't match with actual
default behavior anymore, so we skip those tests in the compatibility
run.
This is safe because the behavior is not changed for existing indices
that don't use synthetic id. Only new indices will have the changed
behavior, so there is no compatibility issue.
To make this work we nedd to make it possible to skip compatibility
tests with overloaded names.
If the name of the test is also a name of an operation such as `get` or
`delete` in below examples, then the build would fail with
```
Execution failed for task ':rest-api-spec:yamlRestCompatTestTransform'.
> class com.fasterxml.jackson.databind.node.ObjectNode cannot be cast to
class com.fasterxml.jackson.databind.node.ArrayNode
```
Instead of throwing an (assert) exception if the node to transform
(skip) is of the wrong type, we now just ignore it instead.
Example of test that could not be skipped before, but that can be
skipped now:
```
get:
- do:
get:
index: id_generation_test
id: cZZNs7B9sSWsyrL5AAABeRnSA5M
```
---
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> and Cursor
* Enable synthetic_id by default if settings are valid
Newly created time_series indices use synthetic id by default.
The new IndexVersion, TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT, prevent old indices
(with older index version) from changing default behavior.
`index.mapping.synthetic_id` defaults to true if
- Feature flag TSDB_SYNTHETIC_ID_FEATURE_FLAG is enabled
- index mode is time_series
- index version is TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT or later
- codec is default or unset
and setting is still allowed to be explicitly set if index version
is TIME_SERIES_USE_SYNTHETIC_ID_94 or later. Only the default behavior
is controlled by TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT.
This default logic is duplicated by IndexMetadata to make sure metadata always
agree with Settings.
MODE setting moved above SYNTHETIC_ID to allow access.
* NodeFeature index.time_series_synthetic_id_default
Making sure to add this node feature in the same PR as adding the new
index version, `TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT` to make sure the
node feature can be used as a shortcut / replacement for checking
version in tests.
* Skip tsdb/25_id... and delete/70_tsdb in yamlRestCompatTest
The compat tests run with a copy of the yaml file from the BWC branch,
which doesn't contain the skip for index.time_series_synthetic_id.
This means the hardcoded ids in the test doesn't match with actual
default behavior anymore, so we skip those tests in the compatibility
run.
This is safe because the behavior is not changed for existing indices
that don't use synthetic id. Only new indices will have the changed
behavior, so there is no compatibility issue.
To make this work we nedd to make it possible to skip compatibility
tests with overloaded names.
If the name of the test is also a name of an operation such as `get` or
`delete` in below examples, then the build would fail with
```
Execution failed for task ':rest-api-spec:yamlRestCompatTestTransform'.
> class com.fasterxml.jackson.databind.node.ObjectNode cannot be cast to
class com.fasterxml.jackson.databind.node.ArrayNode
```
Instead of throwing an (assert) exception if the node to transform
(skip) is of the wrong type, we now just ignore it instead.
Example of test that could not be skipped before, but that can be
skipped now:
```
get:
- do:
get:
index: id_generation_test
id: cZZNs7B9sSWsyrL5AAABeRnSA5M
```
---
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> and Cursor
Newly created time_series indices use synthetic id by default.
The new IndexVersion,
TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT, prevent old indices (with older index version) from changing default behavior.index.mapping.synthetic_iddefaults to true ifTSDB_SYNTHETIC_ID_FEATURE_FLAGis enabledTIME_SERIES_USE_SYNTHETIC_ID_DEFAULTor laterThe synthetic id setting is still allowed to be explicitly set if index version is
TIME_SERIES_USE_SYNTHETIC_ID_94or later. The new index version,TIME_SERIES_USE_SYNTHETIC_ID_DEFAULT, only controls the default behavior.This logic for if synthetic id should be used or not is duplicated from
IndexSettingstoIndexMetadatato make sure metadata always agree with settings.IndexSettings.MODEis moved aboveSYNTHETIC_IDto allow access.Relates ES-14156