Move the failure store enable flag into the data stream options #113176
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gmarouli
added
>refactoring
:Data Management/Data streams
Contributor
Author
|
@elasticmachine update branch |
Contributor
Author
|
@elasticmachine update branch |
gmarouli
added
auto-backport
gmarouli
commented
Sep 23, 2024
Comment on lines
-62
to
-66
| @Nullable | ||
| public DataStreamFailureStore getFailureStore() { | ||
| return failureStore; | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
Redundant method since the DataStreamOptions is a record.
elasticsearchmachine
added
the
Team:Data Management
Collaborator
|
Pinging @elastic/es-data-management (Team:Data Management) |
gmarouli
commented
Sep 26, 2024
| @Nullable | ||
| private final DataStreamLifecycle lifecycle; | ||
| private final boolean failureStoreEnabled; | ||
| private final DataStreamOptions dataStreamOptions; |
Contributor
Author
There was a problem hiding this comment.
I thought it would be easier to refer to an empty constant than always perform the null check before accessing this object. Only on serialisation we use null to avoid serialising empty configurations.
jbaiera
approved these changes
Sep 27, 2024
Member
jbaiera
left a comment
jbaiera
left a comment
There was a problem hiding this comment.
This looks really great! I only had a couple comments but otherwise LGTM!
server/src/main/java/org/elasticsearch/cluster/metadata/DataStream.java
Outdated
Show resolved
Hide resolved
...s/src/test/java/org/elasticsearch/datastreams/lifecycle/DataStreamLifecycleServiceTests.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@elasticmachine update branch |
Contributor
Author
|
@elasticmachine update branch |
gmarouli
added
the
auto-merge-without-approval
Collaborator
💚 Backport successful
|
gmarouli
added a commit
to gmarouli/elasticsearch
that referenced
this pull request
Sep 30, 2024
elasticsearchmachine
pushed a commit
that referenced
this pull request
Sep 30, 2024
matthewabbott
pushed a commit
to matthewabbott/elasticsearch
that referenced
this pull request
Oct 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As the data stream start having built-in features, for example the data stream lifecycle and in the future the failure store, we would like to group these features under one unified entity.
This entity is going to be the data stream options and the first attempt to move some configuration there has already been implemented, see #109515.
In this PR we refactor the data stream code to use the data stream options instead of a single boolean to enabled the failure store.
The benefit of this are:
Implementation notes:
Currently the record
DataStreamOptionscontains onlyDataStreamFailureStore, and theFailureStoreOptionscontains only theenabledflag.Even though the
enabledflag is nullable and in the future it will possible to configureDataStreamFailureStorewithout setting this flag, currently this is not available because then we would have an emptyDataStreamFailureStoreobject.