Change Slice state into enum

Details

Description

Slice state is currently interacted with as a string. It is IMO not trivial to understand which values it can be compared to, in part because the Replica and Slice states are located in different classes, some repeating same constant names and values.

Also, it's not very clear when does a Slice get into which state and what does that mean.

I think if it's an enum, and documented briefly in the code, it would help interacting with it through code. I don't mind if we include more extensive documentation in the reference guide / wiki and refer people there for more details.

Slice.State declares 4 values: ACTIVE, INACTIVE, CONSTRUCTION, RECOVERY. Are these all the states or did I miss some?

I documented these very briefly, mostly from what I understood from the code, and some chats I had w/ Anshum Gupta. I would definitely appreciate a review on this!

Slice.state is held internally as an enum, but still exposed as a String:

Backwards-compatibility wise, is it OK if we change Slice.getState() to return the enum? It's an API-break, but I assume it's pretty expert and the migration is really easy.

Note that it's still written/read as a String.

I didn't yet get rid of the state constants:

Is it OK to just remove them, or should I deprecate them like I did for STATE?

In this issue I would like to handle Slice, and change Replica separately. After I get some feedback, and if there are no objections, I'll move the rest of the code to use the enum instead of the string.

Shai Erera
added a comment - 30/Mar/15 13:18 Patch implements a basic change, in order to get some feedback first:
Slice.State declares 4 values: ACTIVE, INACTIVE, CONSTRUCTION, RECOVERY. Are these all the states or did I miss some?
I documented these very briefly, mostly from what I understood from the code, and some chats I had w/ Anshum Gupta . I would definitely appreciate a review on this!
Slice.state is held internally as an enum, but still exposed as a String:
Backwards-compatibility wise, is it OK if we change Slice.getState() to return the enum? It's an API-break, but I assume it's pretty expert and the migration is really easy.
Note that it's still written/read as a String.
I didn't yet get rid of the state constants:
Is it OK to just remove them, or should I deprecate them like I did for STATE?
In this issue I would like to handle Slice, and change Replica separately. After I get some feedback, and if there are no objections, I'll move the rest of the code to use the enum instead of the string.

Shalin Shekhar Mangar
added a comment - 30/Mar/15 20:37 Thanks Shai!
Slice.State declares 4 values: ACTIVE, INACTIVE, CONSTRUCTION, RECOVERY. Are these all the states or did I miss some?
No, those are the only ones we have right now.
I documented these very briefly, mostly from what I understood from the code, and some chats I had w/ Anshum Gupta. I would definitely appreciate a review on this!
Looks good. We can expand on this a bit e.g. a shard in construction or recovery state receives indexing requests from the parent shard leader but does not participate in distributed search.
Backwards-compatibility wise, is it OK if we change Slice.getState() to return the enum? It's an API-break, but I assume it's pretty expert and the migration is really easy.
We can change it to an enum everywhere. These are internal/expert APIs so we have leeway here.
Is it OK to just remove them, or should I deprecate them like I did for STATE?
+1 to just remove them.

Shai Erera
added a comment - 30/Mar/15 20:52 Thanks Shalin! I was just about to upload a patch when I noticed your comment, so patch includes:
New State enum replaces all string constants
Moved all the code to use the new enum
Expanded RECOVERY and CONSTRUCTION states' jdoc per Shalin's suggestions.
I also added a CHANGES entry under "Migrating from 5.0" section noting the API change. If you think it's an overkill, I can move the comment under Optimizations.
I will run all the tests tomorrow. Few smoke tests that I picked seem happy with these changes. So would appreciate a review on the changes meanwhile.