Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.2, 6.0
    • Component/s: SolrJ
    • Labels:
      None

      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.

      1. SOLR-7325.patch
        44 kB
        Shai Erera
      2. SOLR-7325.patch
        36 kB
        Shai Erera
      3. SOLR-7325.patch
        4 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Shalin Shekhar Mangar added a comment -

        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.

        Show
        Shalin Shekhar Mangar added a comment - 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.
        Hide
        Shai Erera added a comment -

        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.

        Show
        Shai Erera added a comment - 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.
        Hide
        Shai Erera added a comment -

        Patch fixes more places which got some tests angry. I also replaced some of the strings I found with their CONSTANT reference.

        I think it's ready!

        Show
        Shai Erera added a comment - Patch fixes more places which got some tests angry. I also replaced some of the strings I found with their CONSTANT reference. I think it's ready!
        Hide
        Shai Erera added a comment -

        Shalin Shekhar Mangar (or anyone else), if there are no objections, I'll commit it tomorrow.

        Show
        Shai Erera added a comment - Shalin Shekhar Mangar (or anyone else), if there are no objections, I'll commit it tomorrow.
        Hide
        ASF subversion and git services added a comment -

        Commit 1670566 from Shai Erera in branch 'dev/trunk'
        [ https://svn.apache.org/r1670566 ]

        SOLR-7325: Change Slice state into enum

        Show
        ASF subversion and git services added a comment - Commit 1670566 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1670566 ] SOLR-7325 : Change Slice state into enum
        Hide
        Varun Thacker added a comment -

        Hi Shai Erera,

        Shouldn't the CHANGES entry be under 5.2?

        Show
        Varun Thacker added a comment - Hi Shai Erera , Shouldn't the CHANGES entry be under 5.2?
        Hide
        ASF subversion and git services added a comment -

        Commit 1670582 from Shai Erera in branch 'dev/trunk'
        [ https://svn.apache.org/r1670582 ]

        SOLR-7325: move CHANGES entry under 5.2

        Show
        ASF subversion and git services added a comment - Commit 1670582 from Shai Erera in branch 'dev/trunk' [ https://svn.apache.org/r1670582 ] SOLR-7325 : move CHANGES entry under 5.2
        Hide
        Shai Erera added a comment -

        Good catch Varun Thacker, it was before the 5.1 branch was created. Committed a fix.

        Show
        Shai Erera added a comment - Good catch Varun Thacker , it was before the 5.1 branch was created. Committed a fix.
        Hide
        ASF subversion and git services added a comment -

        Commit 1670596 from Shai Erera in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1670596 ]

        SOLR-7325: Change Slice state into enum

        Show
        ASF subversion and git services added a comment - Commit 1670596 from Shai Erera in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1670596 ] SOLR-7325 : Change Slice state into enum
        Hide
        Shai Erera added a comment -

        Committed to trunk and 5x.

        Show
        Shai Erera added a comment - Committed to trunk and 5x.
        Hide
        ASF subversion and git services added a comment -

        Commit 1676350 from hossman@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1676350 ]

        preemptive cleanup of 'Upgrading' section for 5.2 (SOLR-7325, SOLR-7336, SOLR-4839)

        Show
        ASF subversion and git services added a comment - Commit 1676350 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1676350 ] preemptive cleanup of 'Upgrading' section for 5.2 ( SOLR-7325 , SOLR-7336 , SOLR-4839 )
        Hide
        ASF subversion and git services added a comment -

        Commit 1676351 from hossman@apache.org in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1676351 ]

        preemptive cleanup of 'Upgrading' section for 5.2 (SOLR-7325, SOLR-7336, SOLR-4839 - merge r1676350)

        Show
        ASF subversion and git services added a comment - Commit 1676351 from hossman@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1676351 ] preemptive cleanup of 'Upgrading' section for 5.2 ( SOLR-7325 , SOLR-7336 , SOLR-4839 - merge r1676350)
        Hide
        Anshum Gupta added a comment -

        Bulk close for 5.2.0.

        Show
        Anshum Gupta added a comment - Bulk close for 5.2.0.

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            1 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development