API for adding and removing indices from a data stream by danhermann · Pull Request #79279 · elastic/elasticsearch

danhermann · 2021-10-15T17:39:56Z

This PR includes just the REST endpoint. The service itself was added in previous PRs linked to #75852.

Closes #75852

danhermann · 2021-10-15T18:06:59Z

Pinging @elastic/kibana-stack-management since this is a new API

dakrone

Generally looks good to me! I left a couple of comments but nothing major (I know it's still a draft PR also)

dakrone · 2021-10-15T19:42:44Z

server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamAction.java

+    private String dataStream;
+    private String index;


Since this is not really big enough to be a builder class, I think we should make these final (it's not too hard for a user to pass them in the constructor)

I originally had all the members as final but I believe they have to be mutable so the parser is able to construct them here.

server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamAction.java

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsService.java

elasticmachine · 2021-10-15T21:42:24Z

Pinging @elastic/clients-team (Team:Clients)

elasticmachine · 2021-10-15T21:42:25Z

Pinging @elastic/es-data-management (Team:Data Management)

dakrone

Okay I took another look at this, I left a few comments, hopefully nothing too major.

I was also wondering about documentation, are we planning on doing that in a follow-up PR?

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsService.java

server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamAction.java

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataDataStreamsService.java

...rc/main/java/org/elasticsearch/xpack/datastreams/action/ModifyDataStreamTransportAction.java

...reams/src/main/java/org/elasticsearch/xpack/datastreams/rest/RestModifyDataStreamAction.java

.../plugin/src/yamlRestTest/resources/rest-api-spec/test/data_stream/170_modify_data_stream.yml

…server module, consistent naming

danhermann · 2021-10-16T17:07:24Z

I was also wondering about documentation, are we planning on doing that in a follow-up PR?

Yes, I'll write up the docs in a follow-up PR.

danhermann · 2021-10-16T19:12:48Z

@elasticmachine update branch

dakrone

LGTM, I left one really minor comment that doesn't need another review after addressing.

Out of curiosity, do you think we should (in future work, not time-constrained) support changing which index is the write index of a data stream the same way that we support it for aliases? Does that even make sense for a data stream?

dakrone · 2021-10-19T02:53:14Z

server/src/test/java/org/elasticsearch/action/datastreams/ModifyDataStreamsRequestTests.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class ModifyDataStreamsRequestTests extends AbstractWireSerializingTestCase<Request> {


Can you add an override to mutateInstance for this class also so that it runs the full mutation tests?

danhermann · 2021-10-19T12:00:30Z

@elasticmachine update branch

danhermann · 2021-10-19T12:08:37Z

LGTM, I left one really minor comment that doesn't need another review after addressing.

Thanks, @dakrone!

Out of curiosity, do you think we should (in future work, not time-constrained) support changing which index is the write index of a data stream the same way that we support it for aliases? Does that even make sense for a data stream?

There may be some edge cases in which changing the write index could be useful. Generally speaking, I think it's something that's better avoided since data stream backing indices, under normal usage, will accumulate sequential data in mostly non-overlapping time intervals which facilitates fast retrieval. Allowing arbitrary replacement of the write index could weaken that. It's probably worth a discussion about if we could allow it for the cases where it might be useful without weakening some of the core benefits of data streams.

sethmlarson

Some comments on the API specification:

rest-api-spec/src/main/resources/rest-api-spec/api/indices.modify_data_stream.json

sethmlarson

LGTM

danhermann · 2021-10-19T12:44:11Z

@sethmlarson, thanks for catching the errors in the API spec!

* upstream/master: Validate tsdb's routing_path (elastic#79384) Adjust the BWC version for the return200ForClusterHealthTimeout field (elastic#79436) API for adding and removing indices from a data stream (elastic#79279) Exposing the ability to log deprecated settings at non-critical level (elastic#79107) Convert operator privilege license object to LicensedFeature (elastic#79407) Mute SnapshotBasedIndexRecoveryIT testSeqNoBasedRecoveryIsUsedAfterPrimaryFailOver (elastic#79456) Create cache files with CREATE_NEW & SPARSE options (elastic#79371) Revert "[ML] Use a new annotations index for future annotations (elastic#79151)" [ML] Use a new annotations index for future annotations (elastic#79151) [ML] Removing legacy code from ML/transform auditor (elastic#79434) Fix rate agg with custom `_doc_count` (elastic#79346) Optimize SLM Policy Queries (elastic#79341) Fix execution of exists query within nested queries on field with doc_values disabled (elastic#78841) Stricter UpdateSettingsRequest parsing on the REST layer (elastic#79227) Do not release snapshot file download permit during recovery retries (elastic#79409) Preserve request headers in a mixed version cluster (elastic#79412) Adjust versions after elastic#79044 backport to 7.x (elastic#79424) Mute BulkByScrollUsesAllScrollDocumentsAfterConflictsIntegTests (elastic#79429) Fail on SSPL licensed x-pack sources (elastic#79348) # Conflicts: # server/src/test/java/org/elasticsearch/index/TimeSeriesModeTests.java

REST endpoint for data stream modification service

604ffea

danhermann added >enhancement v8.0.0 :StorageEngine/Data streams Data streams and their lifecycles v7.16.0 labels Oct 15, 2021

danhermann changed the title ~~REST endpoint for data stream modification service~~ API for adding and removing indices from a data stream Oct 15, 2021

danhermann added 2 commits October 15, 2021 12:47

no one defies the checkstyle gods

62c067c

especially not in the server module!

8b40466

sethmlarson added the Team:Clients Meta label for clients team label Oct 15, 2021

danhermann requested a review from dakrone October 15, 2021 18:03

Classify action as non-operator and add stub for YAML test

f485e11

dakrone reviewed Oct 15, 2021

View reviewed changes

danhermann added 2 commits October 15, 2021 15:35

add a test and a comment for IndicesOptions

2bfd2b4

finalize YAML test

3c34705

danhermann marked this pull request as ready for review October 15, 2021 21:42

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Oct 15, 2021

dakrone requested changes Oct 16, 2021

View reviewed changes

danhermann added 4 commits October 16, 2021 06:01

more specific catch in test

4acb612

move request class into action class, move all classes from xpack to …

9861b37

…server module, consistent naming

add unit test for request class

3c26d6b

address remaining review comments

65fdd9d

fix test

027ef7a

Merge branch 'master' into 75852_rest_endpoint

8dba4a9

dakrone approved these changes Oct 19, 2021

View reviewed changes

add mutateInstance test method

8403d45

Merge branch 'master' into 75852_rest_endpoint

2aaafdb

sethmlarson suggested changes Oct 19, 2021

View reviewed changes

API spec comments

50e24f7

sethmlarson approved these changes Oct 19, 2021

View reviewed changes

danhermann merged commit 2e05e3e into elastic:master Oct 19, 2021

danhermann deleted the 75852_rest_endpoint branch October 19, 2021 13:21

danhermann added >feature backport pending and removed >enhancement labels Oct 19, 2021

danhermann added a commit to danhermann/elasticsearch that referenced this pull request Oct 19, 2021

API for adding and removing indices from a data stream (elastic#79279)

6e0a6a5

danhermann mentioned this pull request Oct 19, 2021

[7.x] API for adding and removing indices from a data stream #79481

Merged

danhermann removed the backport pending label Oct 19, 2021

danhermann mentioned this pull request Oct 19, 2021

Adjust test skip version after backport of #79279 #79494

Merged

danhermann added a commit that referenced this pull request Oct 20, 2021

Adjust test skip version after backport of #79279

cbcd629

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Conversation

danhermann commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danhermann commented Oct 15, 2021

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

danhermann Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Oct 15, 2021

Uh oh!

elasticmachine commented Oct 15, 2021

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danhermann commented Oct 16, 2021

Uh oh!

danhermann commented Oct 16, 2021

Uh oh!

dakrone left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

danhermann commented Oct 19, 2021

Uh oh!

danhermann commented Oct 19, 2021

Uh oh!

sethmlarson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sethmlarson left a comment

Choose a reason for hiding this comment

Uh oh!

danhermann commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danhermann commented Oct 15, 2021 •

edited

Loading