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

Pass IndexMetadata to AllocationDecider.canRemain #88453

Conversation

original-brownbear
Copy link
Member

We need the metadata in a number of allocation deciders and pass it to other allocation methods.
Passing it here avoids redundant lookups across deciders.

relates #77466

We need the metadata in a number of allocation deciders and pass it to other allocation methods.
Passing it here avoids redundant lookups across deciders.
@original-brownbear original-brownbear added :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >refactoring v8.4.0 labels Jul 11, 2022
@original-brownbear original-brownbear marked this pull request as ready for review July 11, 2022 20:57
@elasticmachine elasticmachine added the Team:Distributed Meta label for distributed team (obsolete) label Jul 11, 2022
@original-brownbear original-brownbear changed the title Pass IndexMetadata to AllocationDecider.can_remain Pass IndexMetadata to AllocationDecider.canRemain Jul 12, 2022
@DaveCTurner
Copy link
Contributor

Looks like it might make sense to add this to canAllocate too?

@original-brownbear
Copy link
Member Author

Looks like it might make sense to add this to canAllocate too?

Maybe. If we want it I'd do it in a follow-up though. This one is already quite noisy and doing it in canAllocate will be even noisier. I wonder if it's worth it though, canAllocate is obviously not nearly as hot as canRemain and in fact I've never seen it show up much in profiling?

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

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

That is a little surprising - if rebalancing is allowed then every rebalance attempt should call canAllocate on every ⟨index,node⟩ pair. Maybe rebalancing isn't allowed much in these benchmarks?

That said, the balancer uses the canAllocate variant which already accepts an IndexMetadata.

Anyway LGTM.

@original-brownbear
Copy link
Member Author

Thanks David!

Maybe rebalancing isn't allowed much in these benchmarks?

That's what I would expect yes.

@original-brownbear original-brownbear merged commit 4889650 into elastic:master Jul 12, 2022
@original-brownbear original-brownbear deleted the lookup-index-metadata-once branch July 12, 2022 08:53
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Jul 12, 2022
* upstream/master:
  Pass IndexMetadata to AllocationDecider.can_remain (elastic#88453)
  [TSDB] Cache rollup bucket timestamp to reduce rounding cost (elastic#88420)
  Correct some typos/mistakes in comments/docs (elastic#88446)
  Make ClusterInfo use immutable maps in all cases (elastic#88447)
  Reduce map lookups (elastic#88418)
  Don't index geo_shape field in AbstractBuilderTestCase (elastic#88437)
  Remove usages of TestGeoShapeFieldMapperPlugin from enrich module (elastic#88440)
  Fix test memory leak (elastic#88362)
  Improve error when sorting on incompatible types (elastic#88399)
  Remove usages of BucketCollector#getLeafCollector(LeafReaderContext) (elastic#88414)
  Mute ReactiveStorageIT::testScaleWhileShrinking (elastic#88431)
  Clarify snapshot docs on archive indices (elastic#88417)
  [Stack Monitoring] Switch cgroup memory fields to keyword (elastic#88260)
  Fix RealmIdentifier XContent parser (elastic#88410)
  Make LoggedExec gradle task configuration cache compatible (elastic#87621)
  Update CorruptedFileIT so that it passes with new allocation strategy (elastic#88314)
  Update RareClusterStateIT to work with the new shards allocator (elastic#87922)
  Ensure CreateApiKey always creates a new document (elastic#88413)

# Conflicts:
#	x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/v2/RollupShardIndexer.java
@original-brownbear original-brownbear restored the lookup-index-metadata-once branch April 18, 2023 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >refactoring Team:Distributed Meta label for distributed team (obsolete) v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants