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.

Sign up for GitHub

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

Jump to bottom

InternalClusterInfoService should not ignore hidden indices #62995

Merged
merged 5 commits into from
Sep 30, 2020
Merged

InternalClusterInfoService should not ignore hidden indices #62995

merged 5 commits into from
Sep 30, 2020

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Sep 29, 2020

Today InternalClusterInfoService ignores hidden indices when retrieving shard stats of the cluster. This can lead to suboptimal shard allocation decisions as the size of shards are taken into account when allocating new shards or rebalancing existing shards.

@tlrx tlrx added >bug :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v8.0.0 v7.10.0 v7.9.3 labels Sep 29, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team label Sep 29, 2020
@tlrx
Copy link
Member Author

tlrx commented Sep 29, 2020

Note that #52423 moved ML indices to be hidden

original-brownbear
original-brownbear approved these changes Sep 29, 2020
View reviewed changes
Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM (from my end especially with the ML thing out of the way, maybe let Henning double check as well though :))

henningandersen
henningandersen approved these changes Sep 29, 2020
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM2, thanks @tlrx .

I wonder if there is a similar issue for system indices? Or are those captured by this change?

server/src/internalClusterTest/java/org/elasticsearch/cluster/ClusterInfoServiceIT.java Outdated
.put(Store.INDEX_STORE_STATS_REFRESH_INTERVAL_SETTING.getKey(), 0)
.put(EnableAllocationDecider.INDEX_ROUTING_REBALANCE_ENABLE_SETTING.getKey(), EnableAllocationDecider.Rebalance.NONE).build()));

final String indexName = (rarely() ? "." : "") + randomAlphaOfLength(5).toLowerCase(Locale.ROOT);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just use randomBoolean()?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@tlrx
Copy link
Member Author

tlrx commented Sep 29, 2020 •
edited
Loading

I wonder if there is a similar issue for system indices? Or are those captured by this change?

@henningandersen System indices should be correctly resolved using the previous indices options (as long as they are not hidden too), but just in case I pushed 99b262f so that the test is also randomly executed on a system index. Let me know what you think please

@tlrx
Copy link
Member Author

tlrx commented Sep 29, 2020

@elasticmachine update branch

henningandersen
henningandersen approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@henningandersen henningandersen 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 also verifying that this works for system indices.

@tlrx
Copy link
Member Author

tlrx commented Sep 30, 2020

@elasticmachine run elasticsearch-ci/packaging-sample-windows

@tlrx tlrx merged commit 77c4713 into elastic:master Sep 30, 2020
@tlrx tlrx deleted the fix-hidden-indices-in-internal-cluster-info branch September 30, 2020 08:07
@tlrx
Copy link
Member Author

tlrx commented Sep 30, 2020

Thanks Armin & Henning!

@tlrx tlrx mentioned this pull request Sep 30, 2020
Merged
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Sep 30, 2020
@tlrx
InternalClusterInfoService should not ignore hidden indices (elastic#…
bc46bfe 
…62995)

Today InternalClusterInfoService ignores hidden indices when
retrieving shard stats of the cluster. This can lead to suboptimal
shard allocation decisions as the size of shards are taken into
account when allocating new shards or rebalancing existing shards.
@tlrx tlrx mentioned this pull request Sep 30, 2020
Merged
tlrx added a commit that referenced this pull request Sep 30, 2020
@tlrx
InternalClusterInfoService should not ignore hidden indices (#62995) (#…
b099bfb 
…63048)

Today InternalClusterInfoService ignores hidden indices when
retrieving shard stats of the cluster. This can lead to suboptimal
shard allocation decisions as the size of shards are taken into
account when allocating new shards or rebalancing existing shards.
tlrx added a commit that referenced this pull request Sep 30, 2020
@tlrx
InternalClusterInfoService should not ignore hidden indices (#62995) (#…
282d196 
…63049)

Today InternalClusterInfoService ignores hidden indices when
retrieving shard stats of the cluster. This can lead to suboptimal
shard allocation decisions as the size of shards are taken into
account when allocating new shards or rebalancing existing shards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@original-brownbear original-brownbear original-brownbear approved these changes

@henningandersen henningandersen henningandersen approved these changes

Assignees
No one assigned
Labels
>bug :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Meta label for distributed team v7.9.3 v7.10.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tlrx @elasticmachine @original-brownbear @henningandersen @jakelandis