- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
[Failure Store] Conceptually introduce the failure store lifecycle #125258
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.
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
[Failure Store] Conceptually introduce the failure store lifecycle #125258
Conversation
| 
           Pinging @elastic/es-data-management (Team:Data Management)  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left one small comment
| * NOTE that this specifically does not return the write index of the data stream as usually retention | ||
| * is treated differently for the write index (i.e. they first need to be rolled over) | ||
| */ | ||
| public List<Index> getBackingIndicesPastRetention( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method and the one below seem to share most of their logic, would it make sense to pass in the lifecycle and the indices as arguments and deduplicate the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been going back and forth on this. If we do that we will change the intention of the method.
Right now I think it encapsulates a lot of the logic. Meaning, we ask the data stream to figure out which of its backing indices and which of its failure indices should are past retention based on the lifecycle configuration it holds in its internal state.
If we change this to pass the lifecycle and the indices as arguments, we are breaking the encapsulation a bit and it becomes more of a helper method than a Because we could be providing a random list of indices and a random retention. This is not necessarily an issue considering this is only used in one place.
I thought of an intermediate approach. We create a getBackingIndicesPastRetention and we provide a boolean to choose or not choose failure store & the actual retention. This still ensures that indices will belong to the data stream, but it gives us the freedom to define the desired retention and the index component we want. It also allowed me to unify the tests which was a nice plus.
I will include it in the follow up PR because then we can see if it works nicely with the separate retentions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See b87eebf
          💔 Backport failed
 You can use sqren/backport to manually backport by running   | 
    
          💚 All backports created successfully
 Questions ?Please refer to the Backport tool documentation  | 
    
…lastic#125258) * Specify index component when retrieving lifecycle * Add getters for the failure lifecycle * Conceptually introduce the failure store lifecycle (even for now it's the same) (cherry picked from commit 6503c1b) # Conflicts: # modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/DataStreamLifecycleService.java # modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/action/TransportExplainDataStreamLifecycleAction.java # modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/action/TransportGetDataStreamLifecycleStatsAction.java # server/src/main/java/org/elasticsearch/cluster/metadata/ProjectMetadata.java # server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamTests.java # server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamServiceTests.java
…125258) (#125657) * Specify index component when retrieving lifecycle * Add getters for the failure lifecycle * Conceptually introduce the failure store lifecycle (even for now it's the same) (cherry picked from commit 6503c1b) # Conflicts: # modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/DataStreamLifecycleService.java # modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/action/TransportExplainDataStreamLifecycleAction.java # modules/data-streams/src/main/java/org/elasticsearch/datastreams/lifecycle/action/TransportGetDataStreamLifecycleStatsAction.java # server/src/main/java/org/elasticsearch/cluster/metadata/ProjectMetadata.java # server/src/test/java/org/elasticsearch/cluster/metadata/DataStreamTests.java # server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateDataStreamServiceTests.java
…lastic#125258) * Specify index component when retrieving lifecycle * Add getters for the failure lifecycle * Conceptually introduce the failure store lifecycle (even for now it's the same)
In this PR we introduce in the
DataStreamdata structure the concept of the failure lifecycle. Currently, the failure store lifecycle and the data stream lifecycle use exactly the same configuration, but this is the most of the required wiring necessary in theDataStreamLifecycleServiceto support this feature.The changes include:
DataStream. We also split the retrieval of backing and failure indices past retention. This will give us the flexibility to expand on the failure retention as needed in the future.DataStreamLifecycleServiceduring the rollover and the deletion steps.DataStreamTests.javaunderwent a lot of changes because of the change in retrieving the data and failure indices past retention. We also merged the tests for data retention and effective retention because of the extensive overlap in the set-up.