Adding new reindex data stream apis #3518

pquentin · 2025-01-15T05:49:59Z

This is a trick to run validation on #3517.

This adds elasticsearch-specification code for the new data stream reindex API. Specifically:

/_migration/reindex
/_migration/reindex/{index}/_status
/_migration/reindex/{index}/_cancel
/_create_from/{source}/{dest}

github-actions · 2025-01-15T05:51:17Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`migrate.cancel_reindex`	⚪	Missing test	Missing test
`migrate.create_from`	🟠	Missing recording	Missing recording
`migrate.get_reindex_status`	⚪	Missing test	Missing test
`migrate.reindex`	🟠	Missing recording	Missing recording

You can validate these APIs yourself by using the make validate target.

github-actions · 2025-01-15T09:03:49Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`migrate.cancel_reindex`	⚪	Missing test	Missing test
`migrate.create_from`	🟠	Missing recording	Missing recording
`migrate.get_reindex_status`	⚪	Missing test	Missing test
`migrate.reindex`	🟠	Missing recording	Missing recording

You can validate these APIs yourself by using the make validate target.

pquentin

Thanks! I was able to run our YAML test validation locally, and fixed the relevant issues. I also ran make contrib to make validation run, even though it's not super useful for now.

(Also, oops, I'm just realizing that I commented my own PR. Feel free to add your fixes there.)

pquentin · 2025-01-15T13:40:29Z

specification/migrate/create_from/MigrateCreateFromRequest.ts

+ */
+
+import { RequestBase } from '@_types/Base'
+import { CreateFrom } from '../_types/CreateFrom'


Since CreateFrom is only used in this file, it can be included inline. As mentioned in https://github.com/elastic/elasticsearch-specification/blob/main/docs/add-new-api.md:

Try to use less files as possible, for example there is no need to create a custom file for an enum, you can define it in the same file where it's used, unless is a commonly used type.

pquentin · 2025-01-15T13:41:23Z

specification/migrate/create_from/MigrateCreateFromRequest.ts

+    /** The source index or data stream name */
+    source: string
+    /** The destination index or data stream name */
+    dest: string


Suggested change

/** The source index or data stream name */

source: string

/** The destination index or data stream name */

dest: string

/** The source index or data stream name */

source: IndexName

/** The destination index or data stream name */

dest: IndexName

(You will also need to add import { IndexName } from '@_types/common' at the top of the file.)

pquentin · 2025-01-15T13:44:00Z

specification/migrate/create_from/MigrateCreateFromResponse.ts

+export class Response {
+  body: {
+    acknowledged: boolean
+    index: IndexName
+    shards_acknowledged: boolean
+  }
+}


I'm wondering, how/why isn't toXContent used? I know it's not a mistake, as it's confirmed by the YAML tests.

Ha, we knew that question would come up, which is why it's in that comment:

/* * This only exists for the sake of testing the xcontent parser */

This is the request, not the response. The response is just AcknowledgedResponse. The request accepts xcontent. In order to use AbstractXContentSerializingTestCase, we added this toXContent (although for unrelated reasons we later ripped that out and wrote our own xcontent serialization test, and could have moved this toXContent into the unit test).

pquentin · 2025-01-15T13:47:37Z

specification/migrate/get_reindex_status/MigrateGetReindexStatusResponse.ts

+import { AcknowledgedResponseBase } from '@_types/Base'
+
+export class Response {
+  body: AcknowledgedResponseBase


I'm also confused, why isn't toXContent used?

This is a bug in my PR. I'll fix it.

pquentin · 2025-01-15T13:49:21Z

specification/migrate/reindex/MigrateReindexRequest.ts

+ */
+
+import { RequestBase } from '@_types/Base'
+import { MigrateReindex } from '../_types/MigrateReindex'


As above, since MigrateReindex is only used in this file, it can be included inline.

pquentin · 2025-01-15T13:50:56Z

specification/migrate/_types/MigrateReindex.ts

+  /**
+   * Reindex mode. Currently only 'upgrade' is supported.
+   */
+  mode: string


Please use an enum: https://github.com/elastic/elasticsearch-specification/blob/main/docs/modeling-guide.md#enum

pquentin · 2025-01-15T13:52:14Z

specification/migrate/_types/MigrateReindex.ts

+  source: Index
+}
+
+export class Index {


Please use a more specific name, maybe SourceIndex?

pquentin · 2025-01-15T13:54:19Z

specification/migrate/reindex/MigrateReindexResponse.ts

+import { AcknowledgedResponseBase } from '@_types/Base'
+
+export class Response {
+  body: AcknowledgedResponseBase


Again, toXContent does not appear to be used. (And this is confirmed by the YAML tests.)

This is for historical reasons. The Response originally returned information, but was changed to return nothing other than what AcknowledgeResponse returns. I'll fix that in Elasticsearch.
The request implements toXContent, but that's for serialization testing in the unit test only.

Removed in elastic/elasticsearch#120321. I think this PR is correct as-is though (for this).

github-actions · 2025-01-16T20:39:40Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`migrate.cancel_reindex`	⚪	Missing test	Missing test
`migrate.create_from`	🟠	Missing recording	Missing recording
`migrate.get_reindex_status`	⚪	Missing test	Missing test
`migrate.reindex`	🟠	Missing recording	Missing recording

You can validate these APIs yourself by using the make validate target.

github-actions · 2025-01-16T21:57:23Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`migrate.cancel_reindex`	⚪	Missing test	Missing test
`migrate.create_from`	🟠	Missing recording	Missing recording
`migrate.get_reindex_status`	⚪	Missing test	Missing test
`migrate.reindex`	🟠	Missing recording	Missing recording

You can validate these APIs yourself by using the make validate target.

pquentin

Thanks, LGTM! Validation passed locally:

❯ make validate api=migrate.create_from branch=main
Validating endpoints
Validating migrate.create_from
migrate.create_from request has been successfully validated!

✔ 2 out of 2 test request cases are passing.
✔ 2 out of 2 test response cases are passing.

❯ make validate api=migrate.reindex branch=main
Validating endpoints
Validating migrate.reindex
migrate.reindex request has been successfully validated!

✔ 2 out of 2 test request cases are passing.
✔ 2 out of 2 test response cases are passing.

❯ make validate api=migrate.get_reindex_status branch=main
Validating endpoints
Validating migrate.get_reindex_status
migrate.get_reindex_status request has been successfully validated!

✔ 1 out of 1 test request cases are passing.
✔ 1 out of 1 test response cases are passing.

pquentin · 2025-01-17T13:00:54Z

I can't approve my PR. Can you please do it and merge it? Thank you.

Co-authored-by: Keith Massey <[email protected]> (cherry picked from commit b596a79)

This reverts commit b596a79.

This reverts commit 8ef4abf.

masseyke added 3 commits January 14, 2025 14:18

Adding new reindex data stream apis

8fa781c

fixing a typo, and correcting docs

d05ce00

adding more specific types for create_from

64dd485

pquentin requested a review from a team as a code owner January 15, 2025 05:50

github-actions bot added the specification label Jan 15, 2025

pquentin added the backport 8.19 label Jan 15, 2025

pquentin added 2 commits January 15, 2025 09:51

Run make contrib

4c81332

Fix migrate.create_from YAML tests

b2f3bbd

pquentin commented Jan 15, 2025

View reviewed changes

Changes from code review feedback

7dac990

adding correct types in MigrateGetReindexStatusResponse

a548521

pquentin commented Jan 17, 2025

View reviewed changes

masseyke approved these changes Jan 17, 2025

View reviewed changes

masseyke merged commit b596a79 into main Jan 17, 2025
8 checks passed

masseyke deleted the reindex-apis branch January 17, 2025 14:11

github-actions bot pushed a commit that referenced this pull request Jan 17, 2025

Adding new reindex data stream apis (#3518)

3b1ec1a

Co-authored-by: Keith Massey <[email protected]> (cherry picked from commit b596a79)

github-actions bot mentioned this pull request Jan 17, 2025

[Backport 8.x] Adding new reindex data stream apis #3548

Merged

pquentin mentioned this pull request Jan 17, 2025

Adding new reindex data stream apis #3517

Closed

pquentin pushed a commit that referenced this pull request Jan 17, 2025

Adding new reindex data stream apis (#3518) (#3548)

8ef4abf

pquentin added a commit that referenced this pull request Jan 21, 2025

Revert "Adding new reindex data stream apis (#3518)"

2d5ed70

This reverts commit b596a79.

pquentin added a commit that referenced this pull request Jan 21, 2025

Revert "Adding new reindex data stream apis (#3518) (#3548)"

6cd72d5

This reverts commit 8ef4abf.

lcawl mentioned this pull request Jan 22, 2025

Update doc tag for migration APIs #3564

Merged

Adding new reindex data stream apis #3518

Adding new reindex data stream apis #3518

Uh oh!

Conversation

pquentin commented Jan 15, 2025

Uh oh!

github-actions bot commented Jan 15, 2025

Uh oh!

github-actions bot commented Jan 15, 2025

Uh oh!

pquentin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 16, 2025

Uh oh!

github-actions bot commented Jan 16, 2025

Uh oh!

pquentin left a comment

Choose a reason for hiding this comment

Uh oh!

pquentin commented Jan 17, 2025

Uh oh!

Uh oh!

Uh oh!