Skip to content

[Test] Randomly disable sequence numbers in CcrTimeSeriesDataStreamsIT #143930

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tlrx merged 9 commits into from
Mar 13, 2026
Merged

[Test] Randomly disable sequence numbers in CcrTimeSeriesDataStreamsIT #143930

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
866a4cd
[Test] Randomly disable sequence numbers in CcrTimeSeriesDataStreamsIT
tlrx Mar 10, 2026
b0f24eb
Merge branch 'main' into 2026/03/10-disable-seq-no-in-ccrtimeseriesda…
tlrx Mar 11, 2026
bc1b102
reinstate comment
tlrx Mar 11, 2026
602746f
Merge branch 'main' into 2026/03/10-disable-seq-no-in-ccrtimeseriesda…
tlrx Mar 11, 2026
fb43f93
Merge branch 'main' into 2026/03/10-disable-seq-no-in-ccrtimeseriesda…
tlrx Mar 12, 2026
0a8e528
Adjust
tlrx Mar 12, 2026
9ca3ad6
[CI] Auto commit changes from spotless
Mar 12, 2026
fdc5a8f
Merge branch 'main' into 2026/03/10-disable-seq-no-in-ccrtimeseriesda…
tlrx Mar 12, 2026
4ac7c0b
Merge branch 'main' into 2026/03/10-disable-seq-no-in-ccrtimeseriesda…
tlrx Mar 13, 2026
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
16 changes: 9 additions & 7 deletions server/src/main/java/org/elasticsearch/index/IndexSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1020,12 +1020,15 @@ public void validate(Boolean enabled) {
public void validate(Boolean enabled, Map<Setting<?>, Object> settings) {
if (enabled) {
var indexVersion = (IndexVersion) settings.get(SETTING_INDEX_VERSION_CREATED);
if (indexVersion.onOrAfter(IndexVersions.DISABLE_SEQUENCE_NUMBERS) == false
&& indexVersion.equals(IndexVersions.ZERO) == false) {
// We validate settings in different places before a real indexVersion has been assigned or
// is missing for other reasons. In those cases IndexVersion.ZERO is used as fallback value,
// and we don't want to fail those validations. At index creation time we _will_ validate with
// the creation version.
if (indexVersion.equals(IndexVersions.ZERO)) {
// Settings are validated in different places before a real indexVersion has been assigned or is missing for other
// reasons (eg. composable index templates). In those cases IndexVersion.ZERO is used as fallback value, and we
// don't want to fail those validations so we return early here because the next two validation checks require
// the IndexMetadata.SETTING_INDEX_VERSION_CREATED to be set. At index creation time we _will_ validate with the
// creation version.
return;
}
if (indexVersion.onOrAfter(IndexVersions.DISABLE_SEQUENCE_NUMBERS) == false) {
throw new IllegalArgumentException(
String.format(
Locale.ROOT,
Expand All @@ -1036,7 +1039,6 @@ public void validate(Boolean enabled, Map<Setting<?>, Object> settings) {
)
);
}
// Sequence numbers cannot be trimmed for points, so we enforce doc values only usage

@fcofdez fcofdez Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to remove this comment?

@tlrx tlrx Mar 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, I think it is still valid, we can't trim seq no if SEQ_NO_INDEX_OPTIONS_SETTING uses points. But the early exit here means that if a template set SEQ_NO_INDEX_OPTIONS_SETTING it is ignored until the index creation time :(

We could change the SEQ_NO_INDEX_OPTIONS_SETTING to also uses IndexVersion.ZERO but I'm a bit worried about potential impacts :(

@fcofdez fcofdez Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep, Kostas hit the same issue I think #143897 (comment)

@tlrx tlrx Mar 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing this, I'll wait for Kostas pull request to be merged then.

@romseygeek romseygeek Mar 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we want to reinstate the comment here?

@tlrx tlrx Mar 11, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think I misread Francisco's initial comment, yes the comment should be reinstated and should not have been removed in my PR.

I pushed bc1b102

var seqNoIndexOptions = (SeqNoFieldMapper.SeqNoIndexOptions) settings.get(SEQ_NO_INDEX_OPTIONS_SETTING);
if (seqNoIndexOptions != SeqNoFieldMapper.SeqNoIndexOptions.DOC_VALUES_ONLY) {
throw new IllegalArgumentException(
Expand Down
3 changes: 3 additions & 0 deletions .../src/internalClusterTest/java/org/elasticsearch/xpack/ccr/CcrTimeSeriesDataStreamsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,9 @@ private static void putDataStreamTemplate(Client client, String dataStreamName,
if (IndexSettings.TSDB_SYNTHETIC_ID_FEATURE_FLAG) {
settingsBuilder.put(IndexSettings.SYNTHETIC_ID.getKey(), useSyntheticId);
}
if (IndexSettings.DISABLE_SEQUENCE_NUMBERS_FEATURE_FLAG && randomBoolean()) {

@romseygeek romseygeek Mar 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this need to be combined with the doc_values_only flag as well, or is that always set here?

@tlrx tlrx Mar 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Time-series indices default to doc values only in recent versions so it is OK

settingsBuilder.put(IndexSettings.DISABLE_SEQUENCE_NUMBERS.getKey(), true);
}
var putTemplateRequest = new TransportPutComposableIndexTemplateAction.Request(getTestClass().getName().toLowerCase(Locale.ROOT))
.indexTemplate(
ComposableIndexTemplate.builder()
Expand Down
Loading