Skip to content

Conversation

@danhermann
Copy link
Contributor

Adds data streams to cluster state and implements their create, delete, and get operations.

Relates to #53100.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Data streams)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Looks good. I left a number of comments.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Looking good. Left some test comments.

}

static List<DataStream> getDataStreams(ClusterState clusterState, Request request) {
Map<String, DataStream> dataStreams = clusterState.metaData().dataStreams();
Copy link
Member

Choose a reason for hiding this comment

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

unit test?
Also I think this should throw a resource not found exception if a specific data stream doesn't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems there are different existing behaviors when attempting to retrieve a specific resource that does not exist. E.g., aliases and indices return a 404 and index templates and ingest pipelines return an empty list. I don't if those are intentional behavioral differences, but I can certainly return a 404 if that's appropriate for data streams.

Copy link
Member

Choose a reason for hiding this comment

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

I think that returning a 404 in case of requesting a specific data stream is appropriate.

@danhermann danhermann marked this pull request as ready for review March 23, 2020 17:58
@danhermann
Copy link
Contributor Author

@elasticmachine update branch

@martijnvg
Copy link
Member

@elasticmachine update branch

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

@danhermann danhermann merged commit aed8ce7 into elastic:master Mar 24, 2020
@danhermann danhermann deleted the data_streams_crud branch March 24, 2020 11:08
danhermann added a commit to danhermann/elasticsearch that referenced this pull request Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants