Introduce IndexReshardingMetadata #121360

bcully · 2025-01-31T04:01:43Z

This adds to IndexMetadata the persistent state we will need to track while a split is in progress. Nothing outside test code sets it yet, so it doesn't introduce any wire changes yet.

Followups will consult this to make routing decisions and handle backward compatibility if the object is present in metadata.

elasticsearchmachine · 2025-02-13T15:38:11Z

Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing)

This adds to IndexMetadata the persistent state we will need to track while a split is in progress. Nothing outside test code sets it yet, so it doesn't introduce any wire changes yet. Followups will consult this to make routing decisions and handle backward compatibility if the object is present in metadata.

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

Tim-Brooks

Looks pretty good to begin with. I'm glad you were able to implement the core serialization tests.

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

This refactors IndexReshardingMetadata to hold a sum type for persistent state that can be extended with new operations. For instance we will want to add a shrink operation in the future, or we may need to create a new version of split later. Making XContent support polymorphic data felt clunky - perhaps there is a different approach that I'm missing?

Previously it was using a discriminant field to control how to parse the rest of the metadata, which it then did by hand: { "operation": "split", ... } Now it uses a separate key for each kind of metadata, and uses ObjectParser's `declareExclusiveFieldSet` machinery to enforce that only one exists: { "split": { ... } } The down side is that the state field is no longer final, because we don't know which kind of state we have at construction time due to a limitation of the ConstructingObjectParser interface.

henningandersen

Read through the non-test code, left a few comments, otherwise looks good.

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

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

henningandersen · 2025-03-07T11:22:37Z

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

+                assert target == TargetShardState.DONE : "can only move source shard to DONE when all targets are DONE";
+            }
+
+            sourceShards[shardNum] = sourceShardState;


We should not mutate these objects. This is way too dangerous, since the index-metadata is expected to be immutable. Instead we should add a Builder and use this to build a new IndexReshardingState.

Thanks, I agree this is worth doing now. I should admit that I haven't really put a lot of thought into the client API for this state yet, since I thought that would be easier to get right when we were building the consumers. I expect it to churn until we actually have the state machine built into the resharding process.

Done in 0e69a76 (but the API is still just enough to support a unit test for the moment)

henningandersen · 2025-03-07T11:22:56Z

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

+            assert targetShardNum >= 0 && targetShardNum < targetShards.length : "target shardNum is out of bounds";
+            assert targetShards[targetShardNum].ordinal() + 1 == targetShardState.ordinal() : "invalid target shard state transition";
+
+            targetShards[targetShardNum] = targetShardState;


Similar to above, we should make this object immutable.

Done in 0e69a76

henningandersen · 2025-03-07T11:24:14Z

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

+         * @param shardNum a source shard index greater than or equal to 0 and less than the original shard count
+         * @return an array of target shard states in order for the given shard
+         */
+        public TargetShardState[] getTargetStatesFor(int shardNum) {


I think we should either return a Collection of such states - or a Map<int, TargetShardState> to make the shard-id explicit.

I wonder if we even need to return this though or if we can add the methods needed to this class instead? Like the inProgress method below, but related to a specific source shard.

I'll make this private and expose a targetsDone(int sourceShardIdx) or similar instead for the unit test.

Done in 0e69a76

Tim-Brooks

This looks fine to me with mostly minor comments.

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

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

@OverRide

Move constructors up; mark a toXContent @OverRide

ankikuma

Looks good Brendan!

* Introduce IndexReshardingMetadata This adds to IndexMetadata the persistent state we will need to track while a split is in progress. Nothing outside test code sets it yet, so it doesn't introduce any wire changes yet. Followups will consult this to make routing decisions and handle backward compatibility if the object is present in metadata.

elasticsearchmachine added the v9.1.0 label Jan 31, 2025

bcully force-pushed the ES-10565 branch from 612c145 to 231cba2 Compare January 31, 2025 04:20

bcully added the WIP label Jan 31, 2025

bcully force-pushed the ES-10565 branch 6 times, most recently from 2b8d7f0 to 308092b Compare February 13, 2025 00:04

bcully requested review from Tim-Brooks and ankikuma February 13, 2025 00:04

bcully added >non-issue :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. and removed WIP labels Feb 13, 2025

bcully marked this pull request as ready for review February 13, 2025 15:37

elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Feb 13, 2025

bcully force-pushed the ES-10565 branch from 308092b to b61b656 Compare February 13, 2025 15:40

ankikuma reviewed Feb 13, 2025

View reviewed changes

Tim-Brooks reviewed Feb 13, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/IndexReshardingMetadata.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/cluster/metadata/IndexReshardingMetadata.java Outdated Show resolved Hide resolved

bcully added 8 commits February 13, 2025 15:20

Documentation updates (feedback from ankikuma)

790a2d2

Merge branch 'main' of github.com:elastic/elasticsearch into ES-10565

ca5ce65

Merge branch 'main' of github.com:elastic/elasticsearch into ES-10565

4d03d31

Merge branch 'main' into ES-10565

7c20105

Merge branch 'main' of github.com:elastic/elasticsearch into ES-10565

c7f086d

Merge branch 'main' of github.com:elastic/elasticsearch into ES-10565

0d7d1b4

henningandersen reviewed Mar 7, 2025

View reviewed changes

bcully added 3 commits March 7, 2025 11:42

IndexReshardingMetadata PR feedback

0e69a76

Merge branch 'main' of github.com:elastic/elasticsearch into ES-10565

775bd73

IndexReshardingMetadata PR feedback

1d39eb8

Tim-Brooks approved these changes Mar 10, 2025

View reviewed changes

server/src/main/java/org/elasticsearch/cluster/metadata/IndexReshardingMetadata.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/cluster/metadata/IndexReshardingState.java Show resolved Hide resolved

bcully added 2 commits March 10, 2025 14:09

Merge branch 'main' of github.com:elastic/elasticsearch into ES-10565

f6b8db4

IndexReshardingMetadata PR feedback

20264b8

Move constructors up; mark a toXContent @OverRide

ankikuma approved these changes Mar 10, 2025

View reviewed changes

Merge branch 'main' of github.com:elastic/elasticsearch into ES-10565

d0dedd5

bcully merged commit 08b16eb into elastic:main Mar 11, 2025
17 checks passed

bcully deleted the ES-10565 branch March 11, 2025 00:28

Introduce IndexReshardingMetadata #121360

Introduce IndexReshardingMetadata #121360

Uh oh!

Conversation

bcully commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Tim-Brooks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ankikuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bcully commented Jan 31, 2025 •

edited

Loading