Solr
  1. Solr
  2. SOLR-6512

Add a collections API call to add/delete arbitrary properties to a specific replica

    Details

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

      Description

      This is a sub-task for SOLR-6491, but seems generally useful.

      Since this is in support of the "preferredLeader" functionality, I've run into some considerations that I wanted some feedback on how to handle.

      "preferredLeader" has the restriction that there should only be one per slice, so setting this for a particular node means removing the property for all the other replicas on the slice. Not a problem to do, my question is more whether this is something reasonable to enforce on an arbitrary property based on what that property is? Perfectly do-able, but "semantically challenged". Currently, this is never a node with "preferedLeader" set to "false", it is forcibly removed from other nodes in the slice when this property is assigned.

      The problem here is that there's nothing about assigning an arbitrary property to a node that would reasonably imply this kind of behavior. One could always control this with secondary flags on the command, e.g. "shardExclusive=true|false" for instance, perhaps with safety checks in for known one-per-shard properties like "preferredLeader".

      "preferredLeader" seems to fit more naturally into a "role", but currently ADDROLE and DELTEROLE have nothing to do with the notion of setting a role for a particular node relative to a collection/shard. Easy enough to add, but enforcing the "only one node per slice may have this role" rule there is similarly arbitrary and overloads the ADDROLE/DELETEROLE in a way that seems equally confusing. Plus, checking whether the required collection/shard/node params are present becomes based on the value of the property being set, which is all equally arbitrary.

      The other interesting thing is that setting an arbitrary property on a node would allow one to mess things up royally by, say, changing properties like "core", or "base_url" or node_name at will. Actually this is potentially useful, but very, very dangerous and I'm not particularly interested in supporting it . I suppose we could require a prefix, say the only settable properties are "property.whatever".

      We could also add something specific to nodes, something like ADDREPLICAROLE/DELETEREPLICAROLE, perhaps with sub-params like "onlyOneAllowedPerShard", but this gets messy and relies on the users "doing the right thing". I prefer enforcing rules like this based on the role I think. Or at least enforcing these kinds of requirements on the "preferredLeader" role if we go that way.

      What are people's thoughts here? I think I'm tending towards the ADDREPLICAROLE/DELETEREPLICAROLE way of doing this, but it's not set in stone. I have code locally for arbitrary properties that I can modify for the role bits.

      So, if I'm going to summarize the points I'd like feedback on:
      1> Is setting arbitrary properties on a node desirable? If so, should we require a prefix like "property" to prevent resetting values SolrCloud depends on?

      2> Is it better to piggyback on ADDROLE/DELETEROLE? Personally I'm not in favor of this one. Too messy with requiring additional parameters to work right in this case

      3> Is the best option to create new collections API calls for ADDREPLICAROLE/DELETEREPLICAROLE that
      3.1> require collection/slice/node parameters
      3.2> enforces the "onlyOnePerShard" rule for certain known roles
      3.3 v1> allows users to specify arbitrary roles something like "onlyOnePerShard" as an optional T|F parameter, otherwise is totally open.
      or
      3.3 v2> No support other than "preferredLeader", only roles that are pre-defined are allowed, in which case the "onlyOnePerShard" is implicit in the role.

      1. SOLR-6512.patch
        39 kB
        Erick Erickson
      2. SOLR-6512.patch
        38 kB
        Erick Erickson
      3. SOLR-6512.patch
        34 kB
        Erick Erickson
      4. SOLR-6512.patch
        31 kB
        Erick Erickson

        Issue Links

          Activity

          Hide
          Erick Erickson added a comment -

          So after thinking on it for a bit, I decided I like option 3, with the 3.3 v1 variant. I'll attach a preliminary patch in a minute that has ADD/DELETE[REPLICAROLE] collections API commands. It's certainly not ready for committing yet.

          Things I need to do yet:
          1> Right now, it only really recognizes a "preferredLeader" role. I'll remove that restriction.
          2> I'll see add support on the ADDREPLICAROLE for "onePerShard" so if people want to put arbitrary roles in there they can. It'll default to "true". The "preferredLeader" command will barf if onePerShard=false, otherwise it's up to someone adding a new role.

          So the train's leaving the station here, object now if there are problems with this approach please.

          Show
          Erick Erickson added a comment - So after thinking on it for a bit, I decided I like option 3, with the 3.3 v1 variant. I'll attach a preliminary patch in a minute that has ADD/DELETE [REPLICAROLE] collections API commands. It's certainly not ready for committing yet. Things I need to do yet: 1> Right now, it only really recognizes a "preferredLeader" role. I'll remove that restriction. 2> I'll see add support on the ADDREPLICAROLE for "onePerShard" so if people want to put arbitrary roles in there they can. It'll default to "true". The "preferredLeader" command will barf if onePerShard=false, otherwise it's up to someone adding a new role. So the train's leaving the station here, object now if there are problems with this approach please.
          Hide
          Erick Erickson added a comment -

          Well, it took me more than a 'minute'. This patch (haven't checked it over entirely yet) that does the following:

          1> Allows an arbitrary role to be assigned to a particular replica for a collection/slice.
          2> Defaults to 'only one per slice'. Thus if you set the same property on a second node in a slice, it is removed from the first one.
          3> Allows <2> to be overridden by a param multiplePerSlice=true.
          4> Throws an error for <3> if the role in question is in a list of known roles that should have one role per slice. "preferredLeader" is the one and only role in this list at present.

          I'll look at it in the morning and no doubt see stuff I want to change. That said, AFAIK it's quite close to being ready to commit, so speak up now if there are issues.

          Show
          Erick Erickson added a comment - Well, it took me more than a 'minute'. This patch (haven't checked it over entirely yet) that does the following: 1> Allows an arbitrary role to be assigned to a particular replica for a collection/slice. 2> Defaults to 'only one per slice'. Thus if you set the same property on a second node in a slice, it is removed from the first one. 3> Allows <2> to be overridden by a param multiplePerSlice=true. 4> Throws an error for <3> if the role in question is in a list of known roles that should have one role per slice. "preferredLeader" is the one and only role in this list at present. I'll look at it in the morning and no doubt see stuff I want to change. That said, AFAIK it's quite close to being ready to commit, so speak up now if there are issues.
          Hide
          Erick Erickson added a comment -

          Except for CHANGES.txt entries, I think it's ready, assuming that adding an arbitrary role to specific replicase is A Good Thing.

          This patch allows one to add and delete a role on a specific replica. By default there is only one role allowed per slice, with the ability to override by specifying multiplePerSlice=true (defaults to false).

          NOTE: Moving a few static strings around in this patch reflect Shalin's recent work in SOLR-6115: Cleanup enum/string action types in Overseer, OverseerCollectionProcessor and CollectionHandler.

          Show
          Erick Erickson added a comment - Except for CHANGES.txt entries, I think it's ready, assuming that adding an arbitrary role to specific replicase is A Good Thing. This patch allows one to add and delete a role on a specific replica. By default there is only one role allowed per slice, with the ability to override by specifying multiplePerSlice=true (defaults to false). NOTE: Moving a few static strings around in this patch reflect Shalin's recent work in SOLR-6115 : Cleanup enum/string action types in Overseer, OverseerCollectionProcessor and CollectionHandler.
          Hide
          Erick Erickson added a comment -

          Review Board here: https://reviews.apache.org/r/25876/. At least I hope I did it right.

          I'll probably commit this early next week unless there are objections. Mostly waiting that long to give people a chance to comment.

          Show
          Erick Erickson added a comment - Review Board here: https://reviews.apache.org/r/25876/ . At least I hope I did it right. I'll probably commit this early next week unless there are objections. Mostly waiting that long to give people a chance to comment.
          Hide
          Erick Erickson added a comment -

          Mark Miller Noble Paul

          Don't want the train to leave the station without people being on board. This particular patch allows arbitrary roles to be assigned to a replica. The first use-case is, of course, the preferredLeader stuff, but it's not confined to that.

          So we can commit this if it's useful without having to use it for the preferredLeader stuff at all.

          Show
          Erick Erickson added a comment - Mark Miller Noble Paul Don't want the train to leave the station without people being on board. This particular patch allows arbitrary roles to be assigned to a replica. The first use-case is, of course, the preferredLeader stuff, but it's not confined to that. So we can commit this if it's useful without having to use it for the preferredLeader stuff at all.
          Hide
          Yonik Seeley added a comment -

          This particular patch allows arbitrary roles to be assigned to a replica.

          +1 to the functionality in general, I think there will be many uses for this.

          As far as naming though... perhaps this is more about arbitrary metadata/properties, and not specifically roles? Roles could just be a semantic interpretation of specific properties. If we had a setProperty API (or setReplicaProperty), it could be used for both setting roles as well as other things.

          multiplePerSlice=true (defaults to false).

          For a generic API, it feels like the default should be the reverse. sliceUnique=true/false (with the default being false)

          Show
          Yonik Seeley added a comment - This particular patch allows arbitrary roles to be assigned to a replica. +1 to the functionality in general, I think there will be many uses for this. As far as naming though... perhaps this is more about arbitrary metadata/properties, and not specifically roles? Roles could just be a semantic interpretation of specific properties. If we had a setProperty API (or setReplicaProperty), it could be used for both setting roles as well as other things. multiplePerSlice=true (defaults to false). For a generic API, it feels like the default should be the reverse. sliceUnique=true/false (with the default being false)
          Hide
          Erick Erickson added a comment -

          Yeah, I flipped back and forth between roles and properties when I started this. After coding it up as roles, I actually came to think that properties would be less error-prone. It's too easy to foo on string maintenance when you have multiple roles. Seems worth changing. I'll default to removing properties that are set to false in the interest of brevity.

          As far as the sliceUnique defaulting to false, hmmm. I guess that works. How to reconcile the fact that "preferredLeader" is always sliceUnique=true? Silently set sliceUnique=true for properties we know must be sliceUnique=true (I suppose a bit of documentation is in order here)? Fail unless they specify sliceUnique=true? or only fail if they explicitly specify sliceUnique=false?

          I'm tending to silently override sliceUnique for known properties and failing if they explicitly specify sliceUnique=false for properties like "preferredLeader" on the principle that they should be notified that they're trying to do something inconsistent.

          Show
          Erick Erickson added a comment - Yeah, I flipped back and forth between roles and properties when I started this. After coding it up as roles, I actually came to think that properties would be less error-prone. It's too easy to foo on string maintenance when you have multiple roles. Seems worth changing. I'll default to removing properties that are set to false in the interest of brevity. As far as the sliceUnique defaulting to false, hmmm. I guess that works. How to reconcile the fact that "preferredLeader" is always sliceUnique=true? Silently set sliceUnique=true for properties we know must be sliceUnique=true (I suppose a bit of documentation is in order here)? Fail unless they specify sliceUnique=true? or only fail if they explicitly specify sliceUnique=false? I'm tending to silently override sliceUnique for known properties and failing if they explicitly specify sliceUnique=false for properties like "preferredLeader" on the principle that they should be notified that they're trying to do something inconsistent.
          Hide
          Yonik Seeley added a comment -

          I'm tending to silently override sliceUnique for known properties and failing if they explicitly specify sliceUnique=false for properties like "preferredLeader" on the principle that they should be notified that they're trying to do something inconsistent.

          Yep, seems fine, and we get to keep sliceUnique=false (I think true would continually suprise people... setting a prop on one replica automatically removes it from certain other replicas?). It will also lead to a more consistent API if/when we have true node-level properties...

          Show
          Yonik Seeley added a comment - I'm tending to silently override sliceUnique for known properties and failing if they explicitly specify sliceUnique=false for properties like "preferredLeader" on the principle that they should be notified that they're trying to do something inconsistent. Yep, seems fine, and we get to keep sliceUnique=false (I think true would continually suprise people... setting a prop on one replica automatically removes it from certain other replicas?). It will also lead to a more consistent API if/when we have true node-level properties...
          Hide
          Erick Erickson added a comment -

          Reworked patch for arbitrary property assignment rather than roles. Also on review board, see: https://reviews.apache.org/r/26161/

          the "preferredLeader" role is special in that there is a list of known properties that we enforce the "only one per slice" rule. This list may be added to in the future if necessary.

          There is an optional parameter "sliceUnique" that can be specified with arbitrary properties to enforce this rule without changing the code. sliceUnique defaults to "false", in which case properties can be added however desired.

          I'll probably commit this Wednesday unless there are objections.

          Show
          Erick Erickson added a comment - Reworked patch for arbitrary property assignment rather than roles. Also on review board, see: https://reviews.apache.org/r/26161/ the "preferredLeader" role is special in that there is a list of known properties that we enforce the "only one per slice" rule. This list may be added to in the future if necessary. There is an optional parameter "sliceUnique" that can be specified with arbitrary properties to enforce this rule without changing the code. sliceUnique defaults to "false", in which case properties can be added however desired. I'll probably commit this Wednesday unless there are objections.
          Hide
          Erick Erickson added a comment -

          Final patch with CHANGES.txt addition

          Show
          Erick Erickson added a comment - Final patch with CHANGES.txt addition
          Hide
          ASF subversion and git services added a comment -

          Commit 1628773 from Erick Erickson in branch 'dev/trunk'
          [ https://svn.apache.org/r1628773 ]

          SOLR-6512: Add a collections API call to add/delete arbitrary properties to a specific replica

          Show
          ASF subversion and git services added a comment - Commit 1628773 from Erick Erickson in branch 'dev/trunk' [ https://svn.apache.org/r1628773 ] SOLR-6512 : Add a collections API call to add/delete arbitrary properties to a specific replica
          Hide
          ASF subversion and git services added a comment -

          Commit 1628802 from Erick Erickson in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1628802 ]

          SOLR-6512: Add a collections API call to add/delete arbitrary properties to a specific replica

          Show
          ASF subversion and git services added a comment - Commit 1628802 from Erick Erickson in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1628802 ] SOLR-6512 : Add a collections API call to add/delete arbitrary properties to a specific replica
          Hide
          Erick Erickson added a comment -

          I'll be adding to the ref guide momentarily.

          Show
          Erick Erickson added a comment - I'll be adding to the ref guide momentarily.
          Hide
          Erick Erickson added a comment -

          NOTE: SOLR-6513 has a change that preserves custom properties across nodes going up and down that slipped through this JIRA.

          Show
          Erick Erickson added a comment - NOTE: SOLR-6513 has a change that preserves custom properties across nodes going up and down that slipped through this JIRA.
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.

            People

            • Assignee:
              Erick Erickson
              Reporter:
              Erick Erickson
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development