-
Notifications
You must be signed in to change notification settings - Fork 25.4k
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
Pass IndexReshardingMetadata over the wire #124841
Conversation
When I introduced IndexReshardingMetadata I inadvertently only covered local serialization. It was not getting sent or received over the wire. This fixes that.
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
Hi @bcully, I've created a changelog YAML for you. |
public IndexReshardingMetadata getReshardingMetadata() { | ||
return reshardingMetadata; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in fa136c9
Also add a basic BWC serialization test for resharding metadata.
…hardingMetadata-OnTheWire
…hardingMetadata-OnTheWire
…hardingMetadata-OnTheWire
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two comments
server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* 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.
When I introduced IndexReshardingMetadata I inadvertently only covered local serialization. It was not getting sent or received over the wire. This fixes that.