Skip to content
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

Add operations with data streams #257

Merged
merged 5 commits into from
Mar 22, 2023

Conversation

zethuman
Copy link
Contributor

@zethuman zethuman commented Mar 21, 2023

Description

Implemented Data Stream API:

  • Create a data stream
  • Delete a data stream
  • Get all data streams
  • Get specific data stream
  • Get Stats of specific data stream

Planned to add:

  • Ingest data into the data stream
  • Searching a data stream
  • Rollover a data stream

Issues Resolved

Closes [#152]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Good stuff. Needs tests, CHANGELOG, USER_GUIDE.

// Modifications Copyright OpenSearch Contributors. See
// GitHub history for details.

// Licensed to Elasticsearch B.V. under one or more contributor
Copy link
Member

Choose a reason for hiding this comment

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

You don't need an ES license grant for new code, remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

// ----- API Definition -------------------------------------------------------

// IndicesCreateDataStream creates a data stream.
//
Copy link
Member

Choose a reason for hiding this comment

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

Remove these many empty commented lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

path.WriteString("/")
path.WriteString("_data_stream")
path.WriteString("/")
path.WriteString(r.Name)
Copy link
Member

Choose a reason for hiding this comment

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

This path is hard-coded except for r.Name, can we collapse into 2 WriteString?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Rakhat Zhuman <zhumanrakhat01@gmail.com>
@zethuman zethuman requested review from dblock and removed request for VachaShah and VijayanB March 21, 2023 17:44
@dblock
Copy link
Member

dblock commented Mar 21, 2023

Looks like some failures in tests, but otherwise looks good!

@zethuman
Copy link
Contributor Author

zethuman commented Mar 22, 2023

@dblock yes, my bad, did not look at the instructions on the Makefile. It seems that I wrote not unit tests, so thinking about adding integration tags, since I need an opensearch instance for the test. What do you think?

@dblock dblock merged commit 8c4dbc6 into opensearch-project:main Mar 22, 2023
@zethuman zethuman deleted the feature/datastream branch April 4, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants