Skip to content

Conversation

@mhl-b
Copy link
Contributor

@mhl-b mhl-b commented Jul 9, 2025

This change splits AllocationDecider into collection of interfaces. This should help with applying deciders that implement interfaces rather than calling all of them with default "Decision.ALWAYS" result. Every decider now explicitly tell which interface it implements:

public class DiskThresholdDecider extends AllocationDecider {
// become
public class DiskThresholdDecider
    implements
        AllocationDecider.ShardToNode,
        AllocationDecider.ForceDuringReplace,
        AllocationDecider.ShardRemain {

So AllocationDeciders can use this type information to group deciders for specific API call.

    public Decision canAllocate(ShardRouting shardRouting, RoutingAllocation allocation) {
        return withDeciders(
            allocation,
            shardToClusterDeciders, // explicitly pass which deciders to use
            decider -> decider.canAllocate(shardRouting, allocation),
            (decider, decision) -> Strings.format("Can not allocate [%s] on any node. [%s]: %s", shardRouting, decider, decision)
        );
    }

For testing ease added a new class DefaultAllocationDecider that uses previous behaviour of abstract class with "Decision.ALWAYS".

Major change in AllocationDecider and AllocationDeciders. I do preserve default behaviour. Especially for ShardToNode decider that combines canAllocate, canForceAllocatePrimary, and canAllocateReplicaWhenThereIsRetentionLease, since later two depends on canAllocate.

@mhl-b mhl-b added >non-issue :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0 labels Jul 9, 2025
@elasticsearchmachine
Copy link
Collaborator

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

import java.util.Optional;
import java.util.Set;

public class DefaultAllocationDecider
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably move to test package. Production deciders implement interfaces. This one only for testing.

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.

I don't like this much, it seems much more complicated and the benefits are pretty minimal. It makes the job of someone implementing a decider (often from outside the distrib/allocation team these days) even more complicated than it is already.

@mhl-b
Copy link
Contributor Author

mhl-b commented Jul 9, 2025

Sad to hear. I'm still liking how it looks, explicit and yet minimal, easier to read and navigate, no hidden defaults. Some deciders are orthogonal to each other but share same defaults, which is counter intuitive. I will keep this PR open for now.

@DiannaHohensee
Copy link
Contributor

This significantly complicates the AllocationDeciders. Adding a new AllocationDecider method would require creating a new interface.

If we were to prove that the current code has significant performance loss in the AllocationDeciders calls, compared to your PoC, then paying the complexity cost could be of interest. But without that proof I think this proposal only has downsides.

@mhl-b
Copy link
Contributor Author

mhl-b commented Jul 10, 2025

Adding a new AllocationDecider method would require creating a new interface.

It does not. Adding new method to AllocationDecider requires new interface. Implementation of decider chooses a set of interfaces it wants to implement.

@mhl-b
Copy link
Contributor Author

mhl-b commented Jul 10, 2025

even more complicated

complexity cost

I see this as perceived complexity, due to more verbose "implement" statement. But it's explicit what each decider suppose to do. Real complexity reduces since every decider has smaller surface, no defaults.

I dont see reasons for these defaults to exists. AllocationDeciders uses "YES" by default when looping through it's deciders. Changing default to something non-yes, will break everything.

@mhl-b mhl-b closed this Jul 17, 2025
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) >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants