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

IndexShardRoutingTable should always be nonempty #105720

Conversation

DaveCTurner
Copy link
Contributor

There's only one remaining test that creates an empty
IndexShardRoutingTable. This commit fixes that, and then adds an
assertion to enforce that all IndexShardRoutingTable instances include
at least a primary shard.

There's only one remaining test that creates an empty
`IndexShardRoutingTable`. This commit fixes that, and then adds an
assertion to enforce that all `IndexShardRoutingTable` instances include
at least a primary shard.
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v8.14.0 labels Feb 22, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team (obsolete) label Feb 22, 2024
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM thanks for cleaning this up.

I think we can also drop the null check for primary such as this one as a follow-up.

@@ -114,7 +115,8 @@ public class IndexShardRoutingTable {
allShardsStarted = false;
}
}
assert primary != null || shards.isEmpty() : shards;
assert shards.isEmpty() == false : "cannot have an empty shard routing table";
Copy link
Member

Choose a reason for hiding this comment

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

The same assertion is already added in the beginning of the method but without the message. Maybe we can merge them by moving the message to the earlier one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry I forgot I added that for some local tests. Removed in 0e08cd5.

@DaveCTurner
Copy link
Contributor Author

I think we can also drop the null check for primary

Yes good point, there's a few obvious ones, see 0e08cd5.

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Feb 22, 2024
@elasticsearchmachine elasticsearchmachine merged commit c8a35d3 into elastic:main Feb 22, 2024
14 checks passed
@DaveCTurner DaveCTurner deleted the 2024/02/22/IndexShardRoutingTable-nonempty branch February 22, 2024 10:18
@DaveCTurner DaveCTurner restored the 2024/02/22/IndexShardRoutingTable-nonempty branch June 17, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >non-issue Team:Distributed Meta label for distributed team (obsolete) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants