Skip to content

Introduce separate shard limit for frozen shards #71392

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

Conversation

henningandersen
Copy link
Contributor

Frozen indices (partial searchable snapshots) require less heap per
shard and the limit can therefore be raised for those. We pick 3000
frozen shards per frozen data node, since we think 2000 is reasonable
to use in production.

Relates #71042 and #34021

Frozen indices (partial searchable snapshots) require less heap per
shard and the limit can therefore be raised for those. We pick 3000
frozen shards per frozen data node, since we think 2000 is reasonable
to use in production.

Relates elastic#71042
@elasticmachine elasticmachine added Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. Team:Data Management Meta label for data/management team labels Apr 7, 2021
@elasticmachine
Copy link
Collaborator

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

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (Team:Core/Features)

Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

This is so well executed, all I have are small nits. Thanks for making this one so easy to review. My small nits don’t require another round.

private Optional<String> checkShardLimit(int newShards, int newFrozenShards, ClusterState state) {
// we verify the two limits independently. This also means that if they have mixed frozen and other data-roles nodes, such a mixed
// node can have both 1000 normal and 3000 frozen shards. This is the trade-off to keep the simplicity of the counts. We advocate
// against such mixed nodes for production use anyway.
Copy link
Member

Choose a reason for hiding this comment

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

I agree with this, good comment.


private static boolean hasFrozen(DiscoveryNode node) {
final List<DiscoveryNodeRole> dataRoles =
node.getRoles().stream().filter(DiscoveryNodeRole::canContainData).collect(Collectors.toUnmodifiableList());
Copy link
Member

Choose a reason for hiding this comment

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

How about return node.getRoles().contains(DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE);? (And sorry if that doesn’t quite compile, I’m reviewing from my iPad, so no IDE support.)

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, much nicer and did compile, changed in 67e92fd


private static boolean hasNonFrozen(DiscoveryNode node) {
final List<DiscoveryNodeRole> dataRoles =
node.getRoles().stream().filter(DiscoveryNodeRole::canContainData).collect(Collectors.toUnmodifiableList());
Copy link
Member

Choose a reason for hiding this comment

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

return node.getRoles().stream().anyMatch(r -> r.canContainData() && r !=DiscoveryNodeRole.DATA_FROZEN_NODE_ROLE);?

Copy link
Member

Choose a reason for hiding this comment

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

This suggested predicate almost exactly matches the name of the method (it has a non-frozen data role), while the current implementation requires a little bit of though. And we can avoid the unnecessary materialization of a collection, etc.

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, that is nicer, added in 67e92fd

@henningandersen
Copy link
Contributor Author

@elasticmachine update branch

@henningandersen
Copy link
Contributor Author

@elasticmachine update branch

@henningandersen henningandersen merged commit eee399a into elastic:master Apr 15, 2021
henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Apr 15, 2021
Frozen indices (partial searchable snapshots) require less heap per
shard and the limit can therefore be raised for those. We pick 3000
frozen shards per frozen data node, since we think 2000 is reasonable
to use in production.

Relates elastic#71042 and elastic#34021
henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Apr 16, 2021
Frozen indices created on 7.12 would not belong to the frozen shard
limit group, now we convert them when last node is upgraded.

Relates elastic#71392
henningandersen added a commit that referenced this pull request Apr 17, 2021
If master ends up on a newer version than other cluster members, we
cannot apply the new index setting for shard limits. We skip doing so
for now.

Relates #71392
henningandersen added a commit that referenced this pull request Apr 17, 2021
Frozen indices (partial searchable snapshots) require less heap per
shard and the limit can therefore be raised for those. We pick 3000
frozen shards per frozen data node, since we think 2000 is reasonable
to use in production.

Relates #71042 and #34021

Includes #71781 and #71777
henningandersen added a commit that referenced this pull request Apr 20, 2021
Frozen indices created on 7.12 would not belong to the frozen shard
limit group, now we convert them when last node is upgraded.

Relates #71392
henningandersen added a commit that referenced this pull request Apr 20, 2021
Frozen indices created on 7.12 would not belong to the frozen shard
limit group, now we convert them when last node is upgraded.

Relates #71392
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Indices APIs APIs to create and manage indices and templates :Distributed Coordination/Autoscaling :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement Team:Data Management Meta label for data/management team Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants