Updating TransportRolloverAction.checkBlock so that non-write-index blocks do not prevent data stream rollover#122905
Merged
masseyke merged 13 commits intoelastic:mainfrom Feb 20, 2025
Conversation
…locks do not prevent data stream rollover
masseyke
added
>bug
:StorageEngine/Data streams
Collaborator
|
Hi @masseyke, I've created a changelog YAML for you. |
…ticsearch into rollover-action-checkBlock
masseyke
added
v9.0.0
v8.18.0
v8.18.1
v8.19.0
auto-backport
elasticsearchmachine
added
the
Team:Data Management (obsolete)
Collaborator
|
Pinging @elastic/es-data-management (Team:Data Management) |
…ticsearch into rollover-action-checkBlock
parkertimmins
approved these changes
Feb 19, 2025
jbaiera
reviewed
Feb 19, 2025
| ResolvedExpression resolvedRolloverTarget = SelectorResolver.parseExpression(request.getRolloverTarget(), request.indicesOptions()); | ||
| final IndexAbstraction indexAbstraction = state.metadata().getIndicesLookup().get(resolvedRolloverTarget.resource()); | ||
| final String[] indicesToCheck; | ||
| if (indexAbstraction.getType().equals(IndexAbstraction.Type.DATA_STREAM)) { |
Member
There was a problem hiding this comment.
Do we support rollover on data stream aliases? If so, would this get tripped up on one?
Member
Author
There was a problem hiding this comment.
That's a good question! But fortunately this code is in masterOperation: https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/action/admin/indices/rollover/TransportRolloverAction.java#L238-L243:
if (rolloverTargetAbstraction.getType() == IndexAbstraction.Type.ALIAS && rolloverTargetAbstraction.isDataStreamRelated()) {
listener.onFailure(
new IllegalStateException("Aliases to data streams cannot be rolled over. Please rollover the data stream itself.")
);
return;
}
jbaiera
approved these changes
Feb 20, 2025
| assertNotNull(transportRolloverAction.checkBlock(rolloverRequest, clusterState)); | ||
| } | ||
| { | ||
| // Make sure checkBlock returns an exception when failure store non-write indices has a block |
Member
There was a problem hiding this comment.
Suggested change
| // Make sure checkBlock returns an exception when failure store non-write indices has a block | |
| // Make sure checkBlock returns null when failure store non-write indices has a block |
Member
Author
There was a problem hiding this comment.
Oops good catch. Also the verb is wrong in that comment. Fixing both.
Collaborator
💔 Backport failed
You can use sqren/backport to manually backport by running |
masseyke
added a commit
to masseyke/elasticsearch
that referenced
this pull request
Feb 20, 2025
…locks do not prevent data stream rollover (elastic#122905)
masseyke
added a commit
to masseyke/elasticsearch
that referenced
this pull request
Feb 20, 2025
…locks do not prevent data stream rollover (elastic#122905)
masseyke
added a commit
to masseyke/elasticsearch
that referenced
this pull request
Feb 20, 2025
…locks do not prevent data stream rollover (elastic#122905)
afoucret
pushed a commit
to afoucret/elasticsearch
that referenced
this pull request
Feb 21, 2025
…locks do not prevent data stream rollover (elastic#122905)
nielsbauman
added a commit
to nielsbauman/elasticsearch
that referenced
this pull request
Mar 20, 2025
Since elastic#122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. This PR restores that to it's original behavior (which is to return a 400 - illegal argument exception). Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior.
nielsbauman
added a commit
to nielsbauman/elasticsearch
that referenced
this pull request
Mar 20, 2025
Since elastic#122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. This PR restores that to it's original behavior (which is to return a 400 - illegal argument exception). Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior.
nielsbauman
added a commit
that referenced
this pull request
Mar 22, 2025
Since #122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior.
nielsbauman
added a commit
to nielsbauman/elasticsearch
that referenced
this pull request
Mar 22, 2025
Since elastic#122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior. (cherry picked from commit fdd4537) # Conflicts: # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java
nielsbauman
added a commit
to nielsbauman/elasticsearch
that referenced
this pull request
Mar 22, 2025
Since elastic#122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior. (cherry picked from commit fdd4537) # Conflicts: # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 22, 2025
) Since #122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior. (cherry picked from commit fdd4537) # Conflicts: # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 22, 2025
) Since #122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior. (cherry picked from commit fdd4537) # Conflicts: # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 22, 2025
) Since #122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior. (cherry picked from commit fdd4537) # Conflicts: # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverService.java
omricohenn
pushed a commit
to omricohenn/elasticsearch
that referenced
this pull request
Mar 28, 2025
Since elastic#122905 we were throwing NPEs (i.e. 5xxs) when a rollover request has an unknown/non-existent target. Before that, we returned a 400 - illegal argument exception. We now return a 404 which matches "missing target" better. Additionally, to avoid this from happening again, we add a YAML test that asserts the correct exception behavior.
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
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.
Right now, TransportRolloverAction's checkBlock returns an exception if any backing index has a block. If a user puts a block on some old index, the data stream cannot rollover, even though rollover only impacts the write index (and the to-be-created write index). This changes TransportRolloverAction.checkBlock so that rollover doesn't fail if non-write-indices have a block.