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

Adding reindex data stream rest action #118109

Conversation

masseyke
Copy link
Member

@masseyke masseyke commented Dec 5, 2024

This adds the rest endpoint that will be used to reindex a data stream for upgrade. An example request will look like:

POST /_migration/reindex
{
  "mode": "upgrade",
  "source": {
    "index": "my-data-stream"
  }
}

with the response immediately returning the persistent task id, like:

{
  "task" : "reindex-data-stream-my-data-stream"
}

This builds on the transport action and persistent task from #117927. The APIs to query the status of this task and to cancel this task are coming in future PRs.

@masseyke masseyke marked this pull request as ready for review December 5, 2024 21:44
@masseyke masseyke requested a review from dakrone December 5, 2024 21:44
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, I left some minor comments, but they're all nothing major.

@@ -70,22 +81,55 @@ public boolean equals(Object other) {

}

public static class ReindexDataStreamRequest extends ActionRequest {
public static class ReindexDataStreamRequest extends ActionRequest implements IndicesRequest, ToXContent {
private final String mode;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the mode an enum for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did that and then undid it because I was worried about deserialization failures when we add modes in the future. But maybe failing fast and/or dealing with that with a transport version check is the way to go.

});

private static final ConstructingObjectParser<String, Void> SOURCE_PARSER = new ConstructingObjectParser<>(
"source",
Copy link
Member

Choose a reason for hiding this comment

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

We should pull this (and the ParseField("index") and "mode" below) into public static variables so that they are accessible from other classes. It's minor, but it will make it easier to refactor if we decide to change the fields later.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pulling it into fields, this one can change to

Suggested change
"source",
SOURCE_FIELD.getPreferredName(),

I think

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think that's just a label for the parser and it doesn't matter what it is. But it can't hurt to set it to SOURCE_FIELD.getPreferredName(), so I'll do that.

Comment on lines 181 to 183
builder.field("mode", mode);
builder.startObject("source");
builder.field("index", sourceDataStream);
Copy link
Member

Choose a reason for hiding this comment

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

Same here for using the ParseFields

Comment on lines 59 to 62
if (request.getMode().equals("upgrade") == false) {
listener.onFailure(new IllegalArgumentException("Only mode 'upgrade' is supported"));
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

If we make it an enum, we can move this into the request parsing, so that it doesn't go over the transport before being rejected

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

Still LGTM, I left one more comment

});

private static final ConstructingObjectParser<String, Void> SOURCE_PARSER = new ConstructingObjectParser<>(
"source",
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pulling it into fields, this one can change to

Suggested change
"source",
SOURCE_FIELD.getPreferredName(),

I think

@masseyke masseyke added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) auto-backport Automatically create backport pull requests when merged labels Dec 6, 2024
@elasticsearchmachine elasticsearchmachine merged commit 8107cc9 into elastic:main Dec 8, 2024
16 checks passed
@masseyke masseyke deleted the adding-reindex-data-stream-rest-action branch December 8, 2024 23:56
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

masseyke added a commit to masseyke/elasticsearch that referenced this pull request Dec 8, 2024
* Adding a _migration/reindex endpoint

* Adding rest api spec and test

* Adding a feature flag for reindex data streams

* updating json spec

* fixing a typo

* Changing mode to an enum

* Moving ParseFields into public static finals

* Commenting out test that leaves task running, until we add a cancel API

* Removing persistent task id from output

* replacing a string with a variable
elasticsearchmachine pushed a commit that referenced this pull request Dec 9, 2024
* Adding a _migration/reindex endpoint

* Adding rest api spec and test

* Adding a feature flag for reindex data streams

* updating json spec

* fixing a typo

* Changing mode to an enum

* Moving ParseFields into public static finals

* Commenting out test that leaves task running, until we add a cancel API

* Removing persistent task id from output

* replacing a string with a variable
elasticsearchmachine pushed a commit that referenced this pull request Dec 10, 2024
This adds a new transport action to get the status of a migration
reindex (started via the API at #118109), and a new rest action to use
it. The rest action accepts the data stream or index name, and returns
the status. For example if the reindex task exists for data stream
`my-data-stream`:

```
GET /_migration/reindex/my-data-stream/_status?pretty
```

returns

```
{
  "start_time" : 1733519098570,
  "complete" : true,
  "total_indices" : 1,
  "total_indices_requiring_upgrade" : 0,
  "successes" : 0,
  "in_progress" : 0,
  "pending" : 0,
  "errors" : [ ]
}
```

If a reindex task does not exist:

```
GET _migration/reindex/my-data-stream/_status?pretty
```

Then a 404 is returned:

```
{
  "error" : {
    "root_cause" : [
      {
        "type" : "resource_not_found_exception",
        "reason" : "No migration reindex status found for [my-data-stream]"
      }
    ],
    "type" : "resource_not_found_exception",
    "reason" : "No migration reindex status found for [my-data-stream]"
  },
  "status" : 404
}
```
masseyke added a commit to masseyke/elasticsearch that referenced this pull request Dec 10, 2024
This adds a new transport action to get the status of a migration
reindex (started via the API at elastic#118109), and a new rest action to use
it. The rest action accepts the data stream or index name, and returns
the status. For example if the reindex task exists for data stream
`my-data-stream`:

```
GET /_migration/reindex/my-data-stream/_status?pretty
```

returns

```
{
  "start_time" : 1733519098570,
  "complete" : true,
  "total_indices" : 1,
  "total_indices_requiring_upgrade" : 0,
  "successes" : 0,
  "in_progress" : 0,
  "pending" : 0,
  "errors" : [ ]
}
```

If a reindex task does not exist:

```
GET _migration/reindex/my-data-stream/_status?pretty
```

Then a 404 is returned:

```
{
  "error" : {
    "root_cause" : [
      {
        "type" : "resource_not_found_exception",
        "reason" : "No migration reindex status found for [my-data-stream]"
      }
    ],
    "type" : "resource_not_found_exception",
    "reason" : "No migration reindex status found for [my-data-stream]"
  },
  "status" : 404
}
```
elasticsearchmachine pushed a commit that referenced this pull request Dec 10, 2024
* Adding get migration reindex status (#118267)

This adds a new transport action to get the status of a migration
reindex (started via the API at #118109), and a new rest action to use
it. The rest action accepts the data stream or index name, and returns
the status. For example if the reindex task exists for data stream
`my-data-stream`:

```
GET /_migration/reindex/my-data-stream/_status?pretty
```

returns

```
{
  "start_time" : 1733519098570,
  "complete" : true,
  "total_indices" : 1,
  "total_indices_requiring_upgrade" : 0,
  "successes" : 0,
  "in_progress" : 0,
  "pending" : 0,
  "errors" : [ ]
}
```

If a reindex task does not exist:

```
GET _migration/reindex/my-data-stream/_status?pretty
```

Then a 404 is returned:

```
{
  "error" : {
    "root_cause" : [
      {
        "type" : "resource_not_found_exception",
        "reason" : "No migration reindex status found for [my-data-stream]"
      }
    ],
    "type" : "resource_not_found_exception",
    "reason" : "No migration reindex status found for [my-data-stream]"
  },
  "status" : 404
}
```

* adding migration reindex actions to OperatorPrivilegesIT
elasticsearchmachine pushed a commit that referenced this pull request Dec 12, 2024
This introduces the migration reindex cancel API, which cancels a
migration reindex task for a given data stream name that was started
with #118109. For example:

```
POST localhost:9200/_migration/reindex/my-data-stream/_cancel?pretty
```

returns

```
{
  "acknowledged" : true
}
```

This cancels the task, and cancels any ongoing reindexing of backing
indices, but does not do any cleanup.
masseyke added a commit to masseyke/elasticsearch that referenced this pull request Dec 13, 2024
This introduces the migration reindex cancel API, which cancels a
migration reindex task for a given data stream name that was started
with elastic#118109. For example:

```
POST localhost:9200/_migration/reindex/my-data-stream/_cancel?pretty
```

returns

```
{
  "acknowledged" : true
}
```

This cancels the task, and cancels any ongoing reindexing of backing
indices, but does not do any cleanup.
@masseyke masseyke removed the backport label Dec 13, 2024
elasticsearchmachine pushed a commit that referenced this pull request Dec 13, 2024
This introduces the migration reindex cancel API, which cancels a
migration reindex task for a given data stream name that was started
with #118109. For example:

```
POST localhost:9200/_migration/reindex/my-data-stream/_cancel?pretty
```

returns

```
{
  "acknowledged" : true
}
```

This cancels the task, and cancels any ongoing reindexing of backing
indices, but does not do any cleanup.
maxhniebergall pushed a commit to maxhniebergall/elasticsearch that referenced this pull request Dec 16, 2024
This introduces the migration reindex cancel API, which cancels a
migration reindex task for a given data stream name that was started
with elastic#118109. For example:

```
POST localhost:9200/_migration/reindex/my-data-stream/_cancel?pretty
```

returns

```
{
  "acknowledged" : true
}
```

This cancels the task, and cancels any ongoing reindexing of backing
indices, but does not do any cleanup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Data Management/Data streams Data streams and their lifecycles >enhancement v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants