-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Remove nesting from multi allocation decision #130844
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,20 +23,18 @@ | |
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * This abstract class defining basic {@link Decision} used during shard | ||
| * allocation process. | ||
| * A {@link Decision} used during shard allocation process. | ||
| * | ||
| * @see AllocationDecider | ||
| */ | ||
| public abstract class Decision implements ToXContent, Writeable { | ||
| public sealed interface Decision extends ToXContent, Writeable permits Decision.Single, Decision.Multi { | ||
|
|
||
| public static final Decision ALWAYS = new Single(Type.YES); | ||
| public static final Decision YES = new Single(Type.YES); | ||
| public static final Decision NO = new Single(Type.NO); | ||
| public static final Decision THROTTLE = new Single(Type.THROTTLE); | ||
| Single ALWAYS = new Single(Type.YES); | ||
| Single YES = new Single(Type.YES); | ||
| Single NO = new Single(Type.NO); | ||
| Single THROTTLE = new Single(Type.THROTTLE); | ||
|
|
||
| /** | ||
| * Creates a simple decision | ||
|
|
@@ -46,40 +44,69 @@ public abstract class Decision implements ToXContent, Writeable { | |
| * @param explanationParams additional parameters for the decision | ||
| * @return new {@link Decision} instance | ||
| */ | ||
| public static Decision single(Type type, @Nullable String label, @Nullable String explanation, @Nullable Object... explanationParams) { | ||
| static Decision single(Type type, @Nullable String label, @Nullable String explanation, @Nullable Object... explanationParams) { | ||
| return new Single(type, label, explanation, explanationParams); | ||
| } | ||
|
|
||
| public static Decision readFrom(StreamInput in) throws IOException { | ||
| static Decision readFrom(StreamInput in) throws IOException { | ||
| // Determine whether to read a Single or Multi Decision | ||
| if (in.readBoolean()) { | ||
| Multi result = new Multi(); | ||
| int decisionCount = in.readVInt(); | ||
| for (int i = 0; i < decisionCount; i++) { | ||
| Decision s = readFrom(in); | ||
| result.decisions.add(s); | ||
| var flag = in.readBoolean(); | ||
| assert flag == false : "nested multi decision is not permitted"; | ||
| var single = readSingleFrom(in); | ||
| result.decisions.add(single); | ||
| } | ||
| return result; | ||
| } else { | ||
| final Type type = Type.readFrom(in); | ||
| final String label = in.readOptionalString(); | ||
| final String explanation = in.readOptionalString(); | ||
| if (label == null && explanation == null) { | ||
| return switch (type) { | ||
| case YES -> YES; | ||
| case THROTTLE -> THROTTLE; | ||
| case NO -> NO; | ||
| }; | ||
| } | ||
| return new Single(type, label, explanation); | ||
| return readSingleFrom(in); | ||
| } | ||
| } | ||
|
|
||
| private static Single readSingleFrom(StreamInput in) throws IOException { | ||
| final Type type = Type.readFrom(in); | ||
| final String label = in.readOptionalString(); | ||
| final String explanation = in.readOptionalString(); | ||
| if (label == null && explanation == null) { | ||
| return switch (type) { | ||
| case YES -> YES; | ||
| case THROTTLE -> THROTTLE; | ||
| case NO -> NO; | ||
| }; | ||
| } | ||
| return new Single(type, label, explanation); | ||
| } | ||
|
|
||
| /** | ||
| * Get the {@link Type} of this decision | ||
| * @return {@link Type} of this decision | ||
| */ | ||
| Type type(); | ||
|
|
||
| /** | ||
| * Get the description label for this decision. | ||
| */ | ||
| @Nullable | ||
| String label(); | ||
|
|
||
| /** | ||
| * Get the explanation for this decision. | ||
| */ | ||
| @Nullable | ||
| String getExplanation(); | ||
|
|
||
| /** | ||
| * Return the list of all decisions that make up this decision | ||
| */ | ||
| List<Decision> getDecisions(); | ||
|
|
||
| /** | ||
| * This enumeration defines the | ||
| * possible types of decisions | ||
| */ | ||
| public enum Type implements Writeable { | ||
| enum Type implements Writeable { | ||
| YES(1), | ||
| THROTTLE(2), | ||
| NO(0); | ||
|
|
@@ -110,45 +137,22 @@ public boolean higherThan(Type other) { | |
| return false; | ||
| } else if (other == NO) { | ||
| return true; | ||
| } else if (other == THROTTLE && this == YES) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } else return other == THROTTLE && this == YES; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. slight nit that this isn't our usual style for if-else statements, skipping the {} brackets.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm intellij auto-format did that and spotless didn't complain either. |
||
| } | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Get the {@link Type} of this decision | ||
| * @return {@link Type} of this decision | ||
| */ | ||
| public abstract Type type(); | ||
|
|
||
| /** | ||
| * Get the description label for this decision. | ||
| */ | ||
| @Nullable | ||
| public abstract String label(); | ||
|
|
||
| /** | ||
| * Get the explanation for this decision. | ||
| */ | ||
| @Nullable | ||
| public abstract String getExplanation(); | ||
| /** | ||
| * @return lowest decision by precedence NO->THROTTLE->YES | ||
| */ | ||
| public static Type min(Type a, Type b) { | ||
| return a.higherThan(b) ? b : a; | ||
| } | ||
|
|
||
| /** | ||
| * Return the list of all decisions that make up this decision | ||
| */ | ||
| public abstract List<Decision> getDecisions(); | ||
| } | ||
|
|
||
| /** | ||
| * Simple class representing a single decision | ||
| */ | ||
| public static class Single extends Decision implements ToXContentObject { | ||
| private final Type type; | ||
| private final String label; | ||
| private final String explanationString; | ||
|
|
||
| record Single(Type type, String label, String explanationString) implements Decision, ToXContentObject { | ||
| /** | ||
| * Creates a new {@link Single} decision of a given type | ||
| * @param type {@link Type} of the decision | ||
|
|
@@ -165,24 +169,13 @@ private Single(Type type) { | |
| * @param explanationParams A set of additional parameters | ||
| */ | ||
| public Single(Type type, @Nullable String label, @Nullable String explanation, @Nullable Object... explanationParams) { | ||
| this.type = type; | ||
| this.label = label; | ||
| if (explanationParams != null && explanationParams.length > 0) { | ||
| this.explanationString = String.format(Locale.ROOT, explanation, explanationParams); | ||
| } else { | ||
| this.explanationString = explanation; | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Type type() { | ||
| return this.type; | ||
| } | ||
|
|
||
| @Override | ||
| @Nullable | ||
| public String label() { | ||
| return this.label; | ||
| this( | ||
| type, | ||
| label, | ||
| explanationParams != null && explanationParams.length > 0 | ||
| ? String.format(Locale.ROOT, explanation, explanationParams) | ||
| : explanation | ||
| ); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -199,29 +192,6 @@ public String getExplanation() { | |
| return this.explanationString; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object object) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you remove this because equality is unused?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. records implement hash/equals/toString https://docs.oracle.com/en/java/javase/17/language/records.html
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I keep toString for bwc. If exact format is not important can remove it too.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a change might affect the allocation explain response? I'd leave it for bwc, as you say. |
||
| if (this == object) { | ||
| return true; | ||
| } | ||
|
|
||
| if (object == null || getClass() != object.getClass()) { | ||
| return false; | ||
| } | ||
|
|
||
| Decision.Single s = (Decision.Single) object; | ||
| return this.type == s.type && Objects.equals(label, s.label) && Objects.equals(explanationString, s.explanationString); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| int result = type.hashCode(); | ||
| result = 31 * result + (label == null ? 0 : label.hashCode()); | ||
| String explanationStr = explanationString; | ||
| result = 31 * result + (explanationStr == null ? 0 : explanationStr.hashCode()); | ||
| return result; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| if (explanationString != null) { | ||
|
|
@@ -254,17 +224,20 @@ public void writeTo(StreamOutput out) throws IOException { | |
| /** | ||
| * Simple class representing a list of decisions | ||
| */ | ||
| public static class Multi extends Decision implements ToXContentFragment { | ||
| record Multi(List<Single> decisions) implements Decision, ToXContentFragment { | ||
|
|
||
| private final List<Decision> decisions = new ArrayList<>(); | ||
| public Multi() { | ||
| this(new ArrayList<>()); | ||
| } | ||
|
|
||
| /** | ||
| * Add a decision to this {@link Multi}decision instance | ||
| * @param decision {@link Decision} to add | ||
| * @return {@link Multi}decision instance with the given decision added | ||
| */ | ||
| public Multi add(Decision decision) { | ||
| decisions.add(decision); | ||
| assert decision instanceof Single; | ||
| decisions.add((Single) decision); | ||
| return this; | ||
| } | ||
|
|
||
|
|
@@ -300,26 +273,6 @@ public List<Decision> getDecisions() { | |
| return Collections.unmodifiableList(this.decisions); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object object) { | ||
| if (this == object) { | ||
| return true; | ||
| } | ||
|
|
||
| if (object == null || getClass() != object.getClass()) { | ||
| return false; | ||
| } | ||
|
|
||
| final Decision.Multi m = (Decision.Multi) object; | ||
|
|
||
| return this.decisions.equals(m.decisions); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return 31 * decisions.hashCode(); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| StringBuilder sb = new StringBuilder(); | ||
|
|
||
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 place could create nested multi decision