- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
Add not-preferred allocation decision #133177
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
Conversation
373d328    to
    b40821a      
    Compare
  
    | 
           Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems pragmatic to me.
Am I right in thinking we chose "soft yes" rather than "soft no" because it was more aligned with existing interpretations of YES/NO? because what we're saying from the WriteLoadConstraintDecider feels more like a soft no than a soft yes to me.
I wonder if the distinction between a soft no and a soft yes will be important one day, or they'll both mean effectively the same thing (i.e. MAYBE).
          
 I'm dont think boolean Preferred will last. My personal opinion is that we need some number value attached to YES, a decision confidence. Especially for non-binary deciders(sounds funny in modern world), like resource based where we have shades of grey rather than clean yes/no. Also not-preferred is a relative term, it serves when only not-preferred options left and we need to choose best one. So this relativity means we need a comparison between decisions, hence some ordering, right? For example: 
  | 
    
          
 Are you asking about   | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the preferred dimension unnecessary, let us keep this one-dimensional.
| 
               | 
          ||
| Single ALWAYS = new Single(Type.YES); | ||
| Single YES = new Single(Type.YES); | ||
| Single YES_NOT_PREFERRED = new Single(Type.YES, Preferred.NO); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a new Type.NOT_PREFERRED, just to keep it simple and not have multi-dimensions here. We can evolve ofc, but starting simple seems ... well simpler. We do not need the other variants of this. And NO not preferred, throttle not preferred etc. does not seem relevant.
This Decision would then also be named NOT_PREFERRED, no need for YES in the name, it is a decision between No and Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to a single dimension.
67cbd43    to
    1dbdc7b      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
| public static AllocationDecision fromDecisionType(Decision.Type type) { | ||
| return switch (type) { | ||
| case YES -> YES; | ||
| case YES, NOT_PREFERRED -> YES; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a todo here, preferably with a JIRA number attached. We need to figure out what to do. Perfectly fine to defer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds new Enum variant for
Decision.Type-NOT_PREFERRED, along with existingNO,THROTTLE, andYES.NOT_PREFERREDis intended for theWriteLoadDeciderto indicate not preferable but possible allocations. It's a soft NO. Also changes wire format to enum ordinals.ES-11998