-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Validation for data stream creation #54083
Validation for data stream creation #54083
Conversation
Pinging @elastic/es-core-features (:Core/Features/Data streams) |
.../src/main/java/org/elasticsearch/action/admin/indices/datastream/CreateDataStreamAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
Outdated
Show resolved
Hide resolved
@elasticmachine run elasticsearch-ci/bwc |
@elasticmachine update branch |
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
server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
Outdated
Show resolved
Hide resolved
@elasticmachine run elasticsearch-ci/1 |
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 a few minor comments to consider.
SortedMap<?, ?> map = aliasAndIndexLookup.subMap(ds.getName() + "-", ds.getName() + "."); // '.' is the char after '-' | ||
if (map.size() != 0) { | ||
throw new IllegalStateException("data stream [" + ds.getName() + | ||
"] could create backing indices that conflict with existing indices or aliases"); |
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.
Should we include the list of conflicting names in the message or at least the first few?
@@ -165,9 +166,11 @@ static ClusterState createDataStream(ClusterState currentState, Request request) | |||
throw new IllegalArgumentException("data_stream [" + request.name + "] already exists"); | |||
} | |||
|
|||
MetaDataCreateIndexService.validateIndexOrAliasName(request.name, |
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.
Should we rename the method to either "validateIndexSpaceName" or "validateAbstractIndexName"? Can be done in a follow-up when the naming discussion is clarified.
@@ -165,9 +166,11 @@ static ClusterState createDataStream(ClusterState currentState, Request request) | |||
throw new IllegalArgumentException("data_stream [" + request.name + "] already exists"); | |||
} | |||
|
|||
MetaDataCreateIndexService.validateIndexOrAliasName(request.name, | |||
(s1, s2) -> new IllegalArgumentException("data_stream [" + s1 + "] " + s2)); |
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.
The other usages of validateIndexOrAliasName
throw a dedicated exception. I am not entirely sure if IllegalArgumentException
results in the same RestStatus and message handling. Could we add this to the rest test suite?
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.
Added a REST test to enforce that the same HTTP 400 response code is returned.
@elasticmachine update branch |
@elasticmachine update branch |
Prevent name conflicts with existing indices or aliases.
Relates to #53100