-
Couldn't load subscription status.
- Fork 25.6k
Rename UnassignedInfo#AllocationStatus to #FailedAllocationStatus #137136
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
base: main
Are you sure you want to change the base?
Rename UnassignedInfo#AllocationStatus to #FailedAllocationStatus #137136
Conversation
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
| * | ||
| * Note, ordering of the enum is important, make sure to add new values | ||
| * at the end and handle version serialization properly. | ||
| */ |
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.
Here's the comment explaining that these are unsuccessful allocation attempts.
There are a LOT of allocation status enums. This name better represents the meaning, as there are no successful enum values. Relates ES-12729
606133c to
d21c5d1
Compare
| * at the end and handle version serialization properly. | ||
| */ | ||
| public enum AllocationStatus implements Writeable { | ||
| public enum FailedAllocationStatus implements Writeable { |
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.
FailedAllocationStatus contradicts some of the variants: FETCHING_SHARD_DATA, DELAYED_ALLOCATION, NO_ATTEMPT, those are not failures. I don't find existing name hard to understand. For me more confusing having Reason and AllocationStatus as two different enums, where AllocationStatus is partially a reason why shard is un-assigned, while Reason is some context of assignment.
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 agree. My interpretation of the two fields is:
Reasonis why the shard becomes unassigned in the first place.AllocationStatusis why the shard remains unassigned after attempt to assign it. It can beNO_ATTEMPTif this has not happend yet.
Also, failed allocation has a different meaning, i.e. the shard failed after being allocated to a data node. The fields UnassignedInfo.failedAllocations, UnassignedInfo.failure and UnassignedInfo.failedNodeIds are used for this scenario.
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.
Oh damn, I didn't even see UnassignedInfo#Reason 😵
There are a LOT of allocation status enums.
This name better represents the meaning,
as there are no successful enum values.
Relates ES-12833
Simple IntelliJ aided rename, but lots of files. Left a comment on the source point for the rename.