Add Unassigned meta data #11653
Add Unassigned meta data #11653
Conversation
| /** | ||
| * Holds additional information as to why the shard is in unassigned state. | ||
| */ | ||
| public class UnassignedMeta implements ToXContent { |
There was a problem hiding this comment.
this should implement org.elasticsearch.common.io.stream.Writeable and then this can all be final as well without the static read write methods
There was a problem hiding this comment.
The fields are final, I opted for this compared to Writeable solution since then I would have needed to create PROTO and such, this class has no meaning outside of shard routing, so this makes the solution simpler I think
There was a problem hiding this comment.
no PROTO needed that is purely optional. you just pass the StreamInput to the ctor which would look like this new UnassignedMeta(streamInput) and everything stays final, no proto required at all. I think we should not have yet another way to serialize and we are moving towards Writeable so this one should implement it as well.
|
left some minor comments mainly structural... good change! |
| @@ -224,6 +246,11 @@ public void readFromThin(StreamInput in) throws IOException { | |||
| state = ShardRoutingState.fromValue(in.readByte()); | |||
|
|
|||
| restoreSource = RestoreSource.readOptionalRestoreSource(in); | |||
| if (in.getVersion().onOrAfter(Version.V_1_7_0)) { | |||
There was a problem hiding this comment.
de we want a default non-null value here?
There was a problem hiding this comment.
please ignore - misread the constructor - seems like a null is a valid value.
|
Awesome change, but it's missing documentation? |
| /** | ||
| * A fresh shard routing, never been assigned. | ||
| */ | ||
| NEW((byte) 0), |
There was a problem hiding this comment.
I wonder if it would be handy, for future changes, to make a distinction between shards that were just created via create index api calls (and thus can be assigned anywhere) and ones that were recovered from a cluster state on disk but still need assigned (for which we want to do some effort and find a copy)
There was a problem hiding this comment.
ok, bitting the bullet mode engaged :)
|
+1 - left a couple of minor comments, plus a suggestion to split the Reason.NEW into "new from API" and recovered. |
|
I implemented the first round of feedback, biggest change is several additional reasons for unassigned (quite a few actually :) ), and now its verified that when a shard is created unassigned, we provide the meta data for it |
|
also, would love to hear if the names I came up with for |
| /** | ||
| * A new shard created as a result of a cluster recovery. | ||
| */ | ||
| NEW_RECOVERY, |
There was a problem hiding this comment.
what is a cluster recovery again? I find this really confusing as a name... not sure about an alternative but maybe you can explain when it happens and I can suggest something
There was a problem hiding this comment.
this happens during a full cluster restart, when the shards are unassigned because they got important as part of the initial cluster recovery process, mmm, maybe I can change NEW_API to NEW, and NEW_RECOVER to CLUSTER_RECOVERY, will try it out
| } | ||
|
|
||
| /** | ||
| * Initializes a new empty index, as if it was created from an API. |
|
LGTM. Left some minor suggestions. |
|
this also LGTM the only thing that I really don't like is the |
|
pushed the first round of better names (not yet for UnassignedMeta), and addressed comments |
|
Namings LGTM. My vote goes to UnassignmentInfo (I like unassignment as it doesn't suggest it is unassigned now, Meta and Info are the same imho). I don't feel strongly about this and I'm good with all of the above as well. |
|
ok with |
Unassigned meta includes additional information as to why a shard is unassigned, this is especially handy when a shard moves to unassigned due to node leaving or shard failure. The additional data is provided as part of the cluster state, and as part of `_cat/shards` API. The additional meta includes the timestamp that the shard has moved to unassigned, allowing us in the future to build functionality such as delay allocation due to node leaving until a copy of the shard is found. closes elastic#11653
|
renamed to UnassignedInfo, I think its the best suggestion so far, will push soonish |
Unassigned meta includes additional information as to why a shard is unassigned, this is especially handy when a shard moves to unassigned due to node leaving or shard failure. The additional data is provided as part of the cluster state, and as part of `_cat/shards` API. The additional meta includes the timestamp that the shard has moved to unassigned, allowing us in the future to build functionality such as delay allocation due to node leaving until a copy of the shard is found. closes #11653
clintongormley
added
:Distributed/Distributed
Unassigned meta includes additional information as to why a shard is unassigned, this is especially handy when a shard moves to unassigned due to node leaving or shard failure.
The additional data is provided as part of the cluster state, and as part of
_cat/shardsAPI.The additional meta includes the timestamp that the shard has moved to unassigned, allowing us in the future to build functionality such as delay allocation due to node leaving until a copy of the shard is found.