Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.8, 6.0
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      In SolrCloud, it is possible for a core to come up in any node , and register itself with an arbitrary slice/coreNodeName. This is a legacy requirement and we would like to make it only possible for Overseer to initiate creation of slice/replicas

      We plan to introduce cluster level properties at the top level

      /cluster-props.json

      {
      "noSliceOrReplicaByCores":true"
      }
      

      If this property is set to true, cores won't be able to send STATE commands with unknown slice/coreNodeName . Those commands will fail at Overseer. This is useful for SOLR-5310 / SOLR-5311 where a core/replica is deleted by a command and it comes up later and tries to create a replica/slice

      1. SOLR-5609_5130.patch
        44 kB
        Noble Paul
      2. SOLR-5609_5130.patch
        42 kB
        Noble Paul
      3. SOLR-5609_5130.patch
        41 kB
        Noble Paul
      4. SOLR-5609_5130.patch
        26 kB
        Noble Paul
      5. SOLR-5609.patch
        12 kB
        Noble Paul
      6. SOLR-5609.patch
        12 kB
        Noble Paul

        Issue Links

          Activity

          Hide
          Noble Paul added a comment -

          There was a discussion to revert SOLR-5311 in favor of an explicit configuration parameter to achieve the same

          Show
          Noble Paul added a comment - There was a discussion to revert SOLR-5311 in favor of an explicit configuration parameter to achieve the same
          Hide
          Mark Miller added a comment - - edited

          I don't really like this design.

          I think the mode that tries to make ZK the truth should end up being the standard mode. We don't want some confusing param for this that looks like an option and controls just how cores register.

          We want something like one option that will become the new default - something name like "legacyCloudMode".

          We could start putting it in as legacyCloudMode=false when we want to default to it. Eventually we would remove the legacy mode in a major release most likely. We can't afford to support it long term IMO.

          Having legacyCloudMode (or whatever it's named) = false should allow the cluster to disable the ability for cores to create collections or remove them and turn on the features that let the cluster use zk as the truth.

          Then we can have further options for more advanced cluster behavior.

          Making ZK the truth should be the default mode when you are not using our legacy mode though.

          Show
          Mark Miller added a comment - - edited I don't really like this design. I think the mode that tries to make ZK the truth should end up being the standard mode. We don't want some confusing param for this that looks like an option and controls just how cores register. We want something like one option that will become the new default - something name like "legacyCloudMode". We could start putting it in as legacyCloudMode=false when we want to default to it. Eventually we would remove the legacy mode in a major release most likely. We can't afford to support it long term IMO. Having legacyCloudMode (or whatever it's named) = false should allow the cluster to disable the ability for cores to create collections or remove them and turn on the features that let the cluster use zk as the truth. Then we can have further options for more advanced cluster behavior. Making ZK the truth should be the default mode when you are not using our legacy mode though.
          Hide
          Mark Miller added a comment -

          The other thing to note, is that we don't have to have this new mode do everything that makes sense to do to make zk the truth initially. It's ability to make zk the truth can improve over time. We won't be limited by the history that are with the legacy mode.

          Show
          Mark Miller added a comment - The other thing to note, is that we don't have to have this new mode do everything that makes sense to do to make zk the truth initially. It's ability to make zk the truth can improve over time. We won't be limited by the history that are with the legacy mode.
          Hide
          Noble Paul added a comment -

          makes sense to have an omnibus property like "legacyCloudMode" rather than having specific properties for each behavior.

          Show
          Noble Paul added a comment - makes sense to have an omnibus property like "legacyCloudMode" rather than having specific properties for each behavior.
          Hide
          Shalin Shekhar Mangar added a comment -

          We should really be discussing this under SOLR-5096.

          Having legacyCloudMode (or whatever it's named) = false should allow the cluster to disable the ability for cores to create collections or remove them and turn on the features that let the cluster use zk as the truth.

          +1 in general. This also bring up the question of what features are must to have to use zk as the truth. I don't think we can have users invoke core admin CREATE and UNLOAD commands directly. Instead they should use collection APIs such as addReplica and deleteReplica exclusively. These APIs will invoke overseer commands (to assign a coreNodeName) and then invoke the core admin APIs.

          The other thing to note, is that we don't have to have this new mode do everything that makes sense to do to make zk the truth initially. It's ability to make zk the truth can improve over time. We won't be limited by the history that are with the legacy mode.

          I think at a minimum we need to implement this mode plus SOLR-5130 (addReplica API) before we release this. The modifyCollection APIs and other things can come in later.

          What do you think?

          Show
          Shalin Shekhar Mangar added a comment - We should really be discussing this under SOLR-5096 . Having legacyCloudMode (or whatever it's named) = false should allow the cluster to disable the ability for cores to create collections or remove them and turn on the features that let the cluster use zk as the truth. +1 in general. This also bring up the question of what features are must to have to use zk as the truth. I don't think we can have users invoke core admin CREATE and UNLOAD commands directly. Instead they should use collection APIs such as addReplica and deleteReplica exclusively. These APIs will invoke overseer commands (to assign a coreNodeName) and then invoke the core admin APIs. The other thing to note, is that we don't have to have this new mode do everything that makes sense to do to make zk the truth initially. It's ability to make zk the truth can improve over time. We won't be limited by the history that are with the legacy mode. I think at a minimum we need to implement this mode plus SOLR-5130 (addReplica API) before we release this. The modifyCollection APIs and other things can come in later. What do you think?
          Hide
          Noble Paul added a comment -

          Takeaway from my discussion with Mark Miller .

          If legacyCloud=false , delete the "collection1" or "defaultcol" cores when they send the STATE commands

          Show
          Noble Paul added a comment - Takeaway from my discussion with Mark Miller . If legacyCloud=false , delete the "collection1" or "defaultcol" cores when they send the STATE commands
          Hide
          ASF subversion and git services added a comment -

          Commit 1567014 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1567014 ]

          SOLR-5609 Reverting accidental commit

          Show
          ASF subversion and git services added a comment - Commit 1567014 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1567014 ] SOLR-5609 Reverting accidental commit
          Hide
          Noble Paul added a comment -
          • CLUSTERPROP handling moved to Overseer from OCP
          • honors legacyCloud=false
          • addReplica command
          • tests for addReplica
          Show
          Noble Paul added a comment - CLUSTERPROP handling moved to Overseer from OCP honors legacyCloud=false addReplica command tests for addReplica
          Hide
          Noble Paul added a comment -

          more testcases
          re-enabled the DeleteInactiveReplica test which was Ignored earlier

          Show
          Noble Paul added a comment - more testcases re-enabled the DeleteInactiveReplica test which was Ignored earlier
          Hide
          Noble Paul added a comment -

          OverseerCollectionProcessorTest was failing

          Show
          Noble Paul added a comment - OverseerCollectionProcessorTest was failing
          Hide
          Noble Paul added a comment -

          another test added

          Show
          Noble Paul added a comment - another test added
          Hide
          Shalin Shekhar Mangar added a comment -

          I don't think we can have users invoke core admin CREATE and UNLOAD commands directly. Instead they should use collection APIs such as addReplica and deleteReplica exclusively. These APIs will invoke overseer commands (to assign a coreNodeName) and then invoke the core admin APIs.

          Noble and I had an offline discussion about this – The coreadmin CREATE will fail if used directly against a collection in the new mode. The UNLOAD will still work but it won't remove the node from cluster state. This is because the removeReplica API must also call UNLOAD after removing the node from cluster state so the unload has to work independently. I don't see a way around that.

          Show
          Shalin Shekhar Mangar added a comment - I don't think we can have users invoke core admin CREATE and UNLOAD commands directly. Instead they should use collection APIs such as addReplica and deleteReplica exclusively. These APIs will invoke overseer commands (to assign a coreNodeName) and then invoke the core admin APIs. Noble and I had an offline discussion about this – The coreadmin CREATE will fail if used directly against a collection in the new mode. The UNLOAD will still work but it won't remove the node from cluster state. This is because the removeReplica API must also call UNLOAD after removing the node from cluster state so the unload has to work independently. I don't see a way around that.
          Hide
          Mark Miller added a comment -

          I don't really see a problem with that. I think we want to keep this new zk=truth mode subject to change for a few releases and the semantics of any of the commands can still be worked out. The most important part is that you can't create a new collection by creating a new core. Unload is not really tied into that.

          Show
          Mark Miller added a comment - I don't really see a problem with that. I think we want to keep this new zk=truth mode subject to change for a few releases and the semantics of any of the commands can still be worked out. The most important part is that you can't create a new collection by creating a new core. Unload is not really tied into that.
          Hide
          Noble Paul added a comment -

          Commit 1571264 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1571264 ]
          SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API

          Show
          Noble Paul added a comment - Commit 1571264 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1571264 ] SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API
          Hide
          ASF subversion and git services added a comment -

          Commit 1571272 from Noble Paul in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1571272 ]

          SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API

          Show
          ASF subversion and git services added a comment - Commit 1571272 from Noble Paul in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1571272 ] SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API
          Hide
          ASF subversion and git services added a comment -

          Commit 1571276 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1571276 ]

          SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API

          Show
          ASF subversion and git services added a comment - Commit 1571276 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1571276 ] SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API
          Hide
          ASF subversion and git services added a comment -

          Commit 1571279 from Noble Paul in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1571279 ]

          SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API

          Show
          ASF subversion and git services added a comment - Commit 1571279 from Noble Paul in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1571279 ] SOLR-5609 Don't let cores create slices/named replicas , SOLR-5130 addReplica Collections API
          Hide
          Mark Miller added a comment -

          The test in this will not pass locally for me - I get the same failure you see from the jenkins cluster.

          Show
          Mark Miller added a comment - The test in this will not pass locally for me - I get the same failure you see from the jenkins cluster.
          Hide
          Mark Miller added a comment -
          +      try {
           +        if(reader.getZkClient().exists(ZkStateReader.CLUSTER_PROPS,true))
           +          reader.getZkClient().setData(ZkStateReader.CLUSTER_PROPS,ZkStateReader.toJSON(m),true);
           +        else
           +          reader.getZkClient().create(ZkStateReader.CLUSTER_PROPS, ZkStateReader.toJSON(m),CreateMode.PERSISTENT, true);
           +        clusterProps = reader.getClusterProps();
           +      } catch (Exception e) {
           +        log.error("Unable to set cluster property", e);
           +
           +      }
          

          This doesn't seem good? You just log an error and don't return a problem to the client? And it's not handling interrupted exception properly?

          We should also probably use the ensureExists code up front to create the clusterstate prop node rather than do this if / then stuff.

          Can we change this code to be more in line with similar code that is already in?

          Show
          Mark Miller added a comment - + try { + if(reader.getZkClient().exists(ZkStateReader.CLUSTER_PROPS,true)) + reader.getZkClient().setData(ZkStateReader.CLUSTER_PROPS,ZkStateReader.toJSON(m),true); + else + reader.getZkClient().create(ZkStateReader.CLUSTER_PROPS, ZkStateReader.toJSON(m),CreateMode.PERSISTENT, true); + clusterProps = reader.getClusterProps(); + } catch (Exception e) { + log.error("Unable to set cluster property", e); + + } This doesn't seem good? You just log an error and don't return a problem to the client? And it's not handling interrupted exception properly? We should also probably use the ensureExists code up front to create the clusterstate prop node rather than do this if / then stuff. Can we change this code to be more in line with similar code that is already in?
          Hide
          Mark Miller added a comment - - edited

          if (baseUrl.startsWith("http://")) baseUrl = baseUrl.substring(7);

          This doesn't seem right? Why was this added? We can't have special code that looks for http anymore.

          Show
          Mark Miller added a comment - - edited if (baseUrl.startsWith("http://")) baseUrl = baseUrl.substring(7); This doesn't seem right? Why was this added? We can't have special code that looks for http anymore.
          Hide
          Mark Miller added a comment -
          -    Replica parentShardLeader = null;
           -    try {
           -      parentShardLeader = zkStateReader.getLeaderRetry(collectionName, slice, 10000);
           -    } catch (InterruptedException e) {
           -      Thread.currentThread().interrupt();
           -    }
           -
           +    Replica parentShardLeader = clusterState.getLeader(collectionName, slice);
          

          Why was this change made? You have moved to an API that might return null for the leader and it's not handled at all.

          Show
          Mark Miller added a comment - - Replica parentShardLeader = null; - try { - parentShardLeader = zkStateReader.getLeaderRetry(collectionName, slice, 10000); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - + Replica parentShardLeader = clusterState.getLeader(collectionName, slice); Why was this change made? You have moved to an API that might return null for the leader and it's not handled at all.
          Hide
          Mark Miller added a comment -
          -    Map<String, Object> map = ZkNodeProps.makeMap(Overseer.QUEUE_OPERATION, action.toString().toLowerCase(Locale.ROOT));
           +    Map<String, Object> map = ZkNodeProps.makeMap(Overseer.QUEUE_OPERATION, action.toLower());
          

          Why was this done?

          Show
          Mark Miller added a comment - - Map<String, Object> map = ZkNodeProps.makeMap(Overseer.QUEUE_OPERATION, action.toString().toLowerCase(Locale.ROOT)); + Map<String, Object> map = ZkNodeProps.makeMap(Overseer.QUEUE_OPERATION, action.toLower()); Why was this done?
          Hide
          Mark Miller added a comment -
          -  private void handleProp(ZkNodeProps message, NamedList results) throws KeeperException, InterruptedException {
           + /* private void handleProp(ZkNodeProps message, NamedList results) throws KeeperException, InterruptedException {
                String name = message.getStr("name");
                String val = message.getStr("val");
                Map m = zkStateReader.getClusterProps();
           @@ -440,7 +444,7 @@ private void handleProp(ZkNodeProps message, NamedList results) throws KeeperExc
            
            
            
           -  }
           +  }*/
          

          Why comment that out? Shouldn't we just clean up?

          Show
          Mark Miller added a comment - - private void handleProp(ZkNodeProps message, NamedList results) throws KeeperException, InterruptedException { + /* private void handleProp(ZkNodeProps message, NamedList results) throws KeeperException, InterruptedException { String name = message.getStr("name"); String val = message.getStr("val"); Map m = zkStateReader.getClusterProps(); @@ -440,7 +444,7 @@ private void handleProp(ZkNodeProps message, NamedList results) throws KeeperExc - } + }*/ Why comment that out? Shouldn't we just clean up?
          Hide
          Mark Miller added a comment -

          checkStateInZk(cd);

          Do you have testing for that?

          Show
          Mark Miller added a comment - checkStateInZk(cd); Do you have testing for that?
          Hide
          Mark Miller added a comment -

          Also, in checkStateInZk:

          +          if(cd.getName().equals(r.get(ZkStateReader.CORE_NAME_PROP)) && getBaseUrl().equals(r.get(ZkStateReader.BASE_URL_PROP))){
          

          That should use the core node name to match up and not the base url.

          Show
          Mark Miller added a comment - Also, in checkStateInZk: + if(cd.getName().equals(r.get(ZkStateReader.CORE_NAME_PROP)) && getBaseUrl().equals(r.get(ZkStateReader.BASE_URL_PROP))){ That should use the core node name to match up and not the base url.
          Hide
          Noble Paul added a comment -

          Why was this change made? You have moved to an API that might return null for the leader and it's not handled at all.

          I don't remember making that change. May be it an SVN merge issue. I will take a look right away

          Why was this done?

          That toLowerCase() was looking ugly. But I guess we can avoid that too

          Why comment that out? Shouldn't we just clean up?

          Yes. It should have been cleaned up

          Show
          Noble Paul added a comment - Why was this change made? You have moved to an API that might return null for the leader and it's not handled at all. I don't remember making that change. May be it an SVN merge issue. I will take a look right away Why was this done? That toLowerCase() was looking ugly. But I guess we can avoid that too Why comment that out? Shouldn't we just clean up? Yes. It should have been cleaned up
          Hide
          Noble Paul added a comment -

          Do you have testing for that?

          Yes. It is in the DeleteInactiveReplicaTest

          Show
          Noble Paul added a comment - Do you have testing for that? Yes. It is in the DeleteInactiveReplicaTest
          Hide
          Mark Miller added a comment -

          As a general comment, there is a lot of odd spacing and new lines - can we use the std project formatting?

          Show
          Mark Miller added a comment - As a general comment, there is a lot of odd spacing and new lines - can we use the std project formatting?
          Hide
          Noble Paul added a comment -

          That should use the core node name to match up and not the base url.

          There was a problem , the first time the core is created there is no corenodename assigned. Hence I was comparing the BASE_URL && CORENAME

          Show
          Noble Paul added a comment - That should use the core node name to match up and not the base url. There was a problem , the first time the core is created there is no corenodename assigned. Hence I was comparing the BASE_URL && CORENAME
          Hide
          Noble Paul added a comment -

          This doesn't seem good? You just log an error and don't return a problem to the client?

          The problem is , AFAIK , the Overseer does not have a way to return an error to the client

          Show
          Noble Paul added a comment - This doesn't seem good? You just log an error and don't return a problem to the client? The problem is , AFAIK , the Overseer does not have a way to return an error to the client
          Hide
          Mark Miller added a comment -

          I thought the cluster props were managed via the collections api?

          Are you saying that for all of that api, if the call fails, the client learns of it, but for this one, it will just silently fail unless they dig through the cluster logs to notice? I see this code is in the Oveerseer, so internally it's different, but from a user level, that seems wrong.

          Or what am I missing?

          Show
          Mark Miller added a comment - I thought the cluster props were managed via the collections api? Are you saying that for all of that api, if the call fails, the client learns of it, but for this one, it will just silently fail unless they dig through the cluster logs to notice? I see this code is in the Oveerseer, so internally it's different, but from a user level, that seems wrong. Or what am I missing?
          Hide
          Noble Paul added a comment - - edited

          This doesn't seem right? Why was this added? We can't have special code that looks for http anymore.

          I didn't add this either , I guess the last SVN merge screwed up a lot of stuff . I'm looking into the code and removing the wrong additions

          I thought the cluster props were managed via the collections api?

          Yes, the CLUSTERPROP is a collections API

          The CollectionsHandler is just posting this request to Overseer Queue directly. Unlike most of our APIs this one just does not need the help of OCP. The only possible failure is a ZK write fail. Overseer is not able to communicate the error back . If there is a failure we only come to know about it indirectly .

          Show
          Noble Paul added a comment - - edited This doesn't seem right? Why was this added? We can't have special code that looks for http anymore. I didn't add this either , I guess the last SVN merge screwed up a lot of stuff . I'm looking into the code and removing the wrong additions I thought the cluster props were managed via the collections api? Yes, the CLUSTERPROP is a collections API The CollectionsHandler is just posting this request to Overseer Queue directly. Unlike most of our APIs this one just does not need the help of OCP. The only possible failure is a ZK write fail. Overseer is not able to communicate the error back . If there is a failure we only come to know about it indirectly .
          Hide
          ASF subversion and git services added a comment -

          Commit 1571421 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1571421 ]

          SOLR-5609 , removing accidental changes

          Show
          ASF subversion and git services added a comment - Commit 1571421 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1571421 ] SOLR-5609 , removing accidental changes
          Hide
          ASF subversion and git services added a comment -

          Commit 1571428 from Noble Paul in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1571428 ]

          SOLR-5609 , removing accidental changes

          Show
          ASF subversion and git services added a comment - Commit 1571428 from Noble Paul in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1571428 ] SOLR-5609 , removing accidental changes
          Hide
          Noble Paul added a comment -

          Mark Miller

          I'm planning to change the comparing base_url by creating the coreNodeName upfront. I'm going to fix that soon

          Show
          Noble Paul added a comment - Mark Miller I'm planning to change the comparing base_url by creating the coreNodeName upfront. I'm going to fix that soon
          Hide
          Noble Paul added a comment -

          Stop comparing baseUrl and core name use coreNodeName.

          the CollectionsAPIDistributedZkTest.testCollectionsAPI() will randomly switch between legacy and non-legacy modes

          Show
          Noble Paul added a comment - Stop comparing baseUrl and core name use coreNodeName. the CollectionsAPIDistributedZkTest.testCollectionsAPI() will randomly switch between legacy and non-legacy modes
          Hide
          ASF subversion and git services added a comment -

          Commit 1572449 from Noble Paul in branch 'dev/trunk'
          [ https://svn.apache.org/r1572449 ]

          SOLR-5609 use coreNodeName to compare replicas, CollectionsAPIDistributedZkTest.testCollectionsAPI() randomly switches to legacyCloud=false

          Show
          ASF subversion and git services added a comment - Commit 1572449 from Noble Paul in branch 'dev/trunk' [ https://svn.apache.org/r1572449 ] SOLR-5609 use coreNodeName to compare replicas, CollectionsAPIDistributedZkTest.testCollectionsAPI() randomly switches to legacyCloud=false
          Hide
          ASF subversion and git services added a comment -

          Commit 1572530 from Noble Paul in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1572530 ]

          SOLR-5609 use coreNodeName to compare replicas, CollectionsAPIDistributedZkTest.testCollectionsAPI() randomly switches to legacyCloud=false

          Show
          ASF subversion and git services added a comment - Commit 1572530 from Noble Paul in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1572530 ] SOLR-5609 use coreNodeName to compare replicas, CollectionsAPIDistributedZkTest.testCollectionsAPI() randomly switches to legacyCloud=false
          Hide
          Uwe Schindler added a comment -

          Move issue to Solr 4.9.

          Show
          Uwe Schindler added a comment - Move issue to Solr 4.9.
          Hide
          Steve Rowe added a comment -

          Noble Paul, shouldn't this be resolved with a fix version of 4.8?

          Show
          Steve Rowe added a comment - Noble Paul , shouldn't this be resolved with a fix version of 4.8?
          Hide
          Noble Paul added a comment -

          resolved

          Show
          Noble Paul added a comment - resolved

            People

            • Assignee:
              Noble Paul
              Reporter:
              Noble Paul
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development