Skip to content

Pass IndexReshardingMetadata over the wire #124841

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

Merged
merged 9 commits into from
Mar 17, 2025

Conversation

bcully
Copy link
Contributor

@bcully bcully commented Mar 14, 2025

When I introduced IndexReshardingMetadata I inadvertently only covered local serialization. It was not getting sent or received over the wire. This fixes that.

When I introduced IndexReshardingMetadata I inadvertently
only covered local serialization. It was not getting sent or
received over the wire. This fixes that.
@bcully bcully added >bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. v9.1.0 labels Mar 14, 2025
@bcully bcully requested review from Tim-Brooks and ankikuma March 14, 2025 01:03
@bcully bcully self-assigned this Mar 14, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Indexing Meta label for Distributed Indexing team label Mar 14, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Hi @bcully, I've created a changelog YAML for you.

public IndexReshardingMetadata getReshardingMetadata() {
return reshardingMetadata;
}

@Override
public boolean equals(Object o) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should add a check for the IndexReshardingMetadata being equal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is a good point. It did get me to look at which fields are included and there are some others that are serialized but not included in equality comparison (e.g., timestampRange/eventIngestedRange) and I'm not sure I understand whether that's by design or an oversight. But it does seem like resharding metadata should be included regardless. I'll update.

Copy link
Contributor Author

@bcully bcully Mar 14, 2025

Choose a reason for hiding this comment

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

Done in fa136c9

@@ -586,13 +587,18 @@ public IndexMetadata randomCreate(String name) {
for (int i = 0; i < aliasCount; i++) {
builder.putAlias(randomAlias());
}
if (randomBoolean()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need any compatibility checks in this test ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also wondering if we have a cluster where the master node is on a different (older) version and the data node that initiates the reshard is on a newer version, how do we handle that ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also wondering if we have a cluster where the master node is on a different (older) version and the data node that initiates the reshard is on a newer version, how do we handle that ?

I think we should check at the top of an autoshard action if the cluster-wide minimum version is too low to parse resharding metadata, and fail immediately if so. I don't think it goes in this change though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we not need any compatibility checks in this test ?

I haven't found BWC tests for the other fields that are conditionally serialized, but it does seem like a good idea. I'll add something.

Copy link
Contributor Author

@bcully bcully Mar 14, 2025

Choose a reason for hiding this comment

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

Done in fa136c9

Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

two comments

@bcully bcully requested review from Tim-Brooks and ankikuma March 17, 2025 17:44
Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

LGTM

@bcully bcully merged commit cc6a4bb into elastic:main Mar 17, 2025
17 checks passed
@bcully bcully deleted the IndexReshardingMetadata-OnTheWire branch March 17, 2025 18:35
omricohenn pushed a commit to omricohenn/elasticsearch that referenced this pull request Mar 28, 2025
* Pass IndexReshardingMetadata over the wire

When I introduced IndexReshardingMetadata I inadvertently
only covered local serialization. It was not getting sent or
received over the wire. This fixes that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. Team:Distributed Indexing Meta label for Distributed Indexing team v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants