Solr
  1. Solr
  2. SOLR-8075

Leader Initiated Recovery should not stop a leader that participated in an election with all of it's replicas from becoming a valid leader.

    Details

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

      Description

      Currently, because of SOLR-8069, all the replicas in a shard can be put into LIR.

      If you restart such a shard, the valid leader will will win the election and sync with the shard and then be blocked from registering as ACTIVE because it is in LIR.

      I think that is a little wonky because I don't think it even tries another candidate because the leader that cannot publish ACTIVE does not have it's election canceled.

      While SOLR-8069 should prevent this situation, we should add logic to allow a leader that can sync with it's full shard to become leader and publish ACTIVE regardless of LIR.

      1. SOLR-8075.patch
        8 kB
        Mark Miller
      2. SOLR-8075.patch
        8 kB
        Mark Miller
      3. SOLR-8075.patch
        5 kB
        Mark Miller
      4. SOLR-8075.patch
        8 kB
        Mark Miller
      5. SOLR-8075.patch
        7 kB
        Mark Miller
      6. SOLR-8075.patch
        7 kB
        Mark Miller
      7. SOLR-8075.patch
        3 kB
        Mark Miller

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          Patch with some initial exploration.

          Show
          Mark Miller added a comment - Patch with some initial exploration.
          Hide
          Mark Miller added a comment -

          Okay, with this patch, if the proper leader still somehow ends up in LIR, on restart of the shard, if all replicas participate in an election, it will still become leader.

          I'll open another issue into why this test prevents leadership using a safety catch in publish. My worry with that is that it won't cancel the election, but that is a separate issue.

          Show
          Mark Miller added a comment - Okay, with this patch, if the proper leader still somehow ends up in LIR, on restart of the shard, if all replicas participate in an election, it will still become leader. I'll open another issue into why this test prevents leadership using a safety catch in publish. My worry with that is that it won't cancel the election, but that is a separate issue.
          Hide
          Mark Miller added a comment -

          New patch - last one missed my new test.

          Show
          Mark Miller added a comment - New patch - last one missed my new test.
          Hide
          Mark Miller added a comment -

          One more, updated to trunk and cleaned up a bit.

          Show
          Mark Miller added a comment - One more, updated to trunk and cleaned up a bit.
          Hide
          Mark Miller added a comment -

          Patch getting ready for commit - adds a comment and only clears LIR if the leader is in LIR.

          Show
          Mark Miller added a comment - Patch getting ready for commit - adds a comment and only clears LIR if the leader is in LIR.
          Hide
          Mike Drob added a comment -

          Patch LGTM.

          On commit, can you add a comment to waitForReplicasToComeUp explaining what the return value means? It makes sense now, but in six months I'm pretty sure I won't have a clue.

          Show
          Mike Drob added a comment - Patch LGTM. On commit, can you add a comment to waitForReplicasToComeUp explaining what the return value means? It makes sense now, but in six months I'm pretty sure I won't have a clue.
          Hide
          Mark Miller added a comment -

          I'll commit this shortly.

          Show
          Mark Miller added a comment - I'll commit this shortly.
          Hide
          Shalin Shekhar Mangar added a comment -

          +1 LGTM

          Show
          Shalin Shekhar Mangar added a comment - +1 LGTM
          Hide
          ASF subversion and git services added a comment -

          Commit 1706699 from Mark Miller in branch 'dev/trunk'
          [ https://svn.apache.org/r1706699 ]

          SOLR-8075: Leader Initiated Recovery should not stop a leader that participated in an election with all of it's replicas from becoming a valid leader.

          Show
          ASF subversion and git services added a comment - Commit 1706699 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1706699 ] SOLR-8075 : Leader Initiated Recovery should not stop a leader that participated in an election with all of it's replicas from becoming a valid leader.
          Hide
          ASF subversion and git services added a comment -

          Commit 1706701 from Mark Miller in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1706701 ]

          SOLR-8075: Leader Initiated Recovery should not stop a leader that participated in an election with all of it's replicas from becoming a valid leader.

          Show
          ASF subversion and git services added a comment - Commit 1706701 from Mark Miller in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1706701 ] SOLR-8075 : Leader Initiated Recovery should not stop a leader that participated in an election with all of it's replicas from becoming a valid leader.
          Hide
          Mark Miller added a comment -

          Thanks for taking a look guys.

          Show
          Mark Miller added a comment - Thanks for taking a look guys.
          Hide
          Mark Miller added a comment -

          There appears to be some race or something here. We need to turn off LIR before we try and publish ACTIVE upon becoming the leader. The test must only pass because somehow the check beats the ACTIVE publish or something.

          Show
          Mark Miller added a comment - There appears to be some race or something here. We need to turn off LIR before we try and publish ACTIVE upon becoming the leader. The test must only pass because somehow the check beats the ACTIVE publish or something.
          Hide
          Ishan Chattopadhyaya added a comment -

          How can I investigate / reproduce this? Is the LeaderInitiatedRecoveryOnShardRestartTest failing under certain circumstances?

          Show
          Ishan Chattopadhyaya added a comment - How can I investigate / reproduce this? Is the LeaderInitiatedRecoveryOnShardRestartTest failing under certain circumstances?
          Hide
          Mark Miller added a comment -

          Patch with fix.

          How can I investigate / reproduce this?

          Don't know. Saw it not working on a real cluster.

          I also saw the shard get stuck with no proper leader still due to the proper leader being in LIR. It seems LIR is still a little buggy. Fixing this should be one step towards alleviating that.

          I'm going to work on a test that indexes while restarting the whole cluster and see if I can reproduce some of those bugs.

          Show
          Mark Miller added a comment - Patch with fix. How can I investigate / reproduce this? Don't know. Saw it not working on a real cluster. I also saw the shard get stuck with no proper leader still due to the proper leader being in LIR. It seems LIR is still a little buggy. Fixing this should be one step towards alleviating that. I'm going to work on a test that indexes while restarting the whole cluster and see if I can reproduce some of those bugs.
          Hide
          Mark Miller added a comment -

          Hmm...this still needs a little tweaking I think.

          Show
          Mark Miller added a comment - Hmm...this still needs a little tweaking I think.
          Hide
          Mark Miller added a comment -

          New patch.

          Show
          Mark Miller added a comment - New patch.
          Hide
          Mark Miller added a comment -

          The latest patch also fixes LIR handling a bit. The way things worked were a little off.

          If you were the first replica to attempt to be leader, LIR would never stop you early. Instead, things would fail when publish ACTIVE was attempted. That is not good, because it will probably prevent the next leader from trying to take over.

          So I have moved all the LIR checking code in ElectionContext to a new spot. It's always checked, first attempted leader or not. And not until we see if the replica could be leader and if all the replicas participated in that election or not.

          Show
          Mark Miller added a comment - The latest patch also fixes LIR handling a bit. The way things worked were a little off. If you were the first replica to attempt to be leader, LIR would never stop you early. Instead, things would fail when publish ACTIVE was attempted. That is not good, because it will probably prevent the next leader from trying to take over. So I have moved all the LIR checking code in ElectionContext to a new spot. It's always checked, first attempted leader or not. And not until we see if the replica could be leader and if all the replicas participated in that election or not.
          Hide
          Mike Drob added a comment -

          Update looks good to me. I know you expanded the comments in checkLIR, but can you add a one-line note before the method is called with something to the effect of "needs to happen synchronously before setLeader(true) to avoid rejection?"

          Show
          Mike Drob added a comment - Update looks good to me. I know you expanded the comments in checkLIR , but can you add a one-line note before the method is called with something to the effect of "needs to happen synchronously before setLeader(true) to avoid rejection?"
          Hide
          Mark Miller added a comment -

          It's not that it needs to happen before setLeader. The pertinent move it putting it above the super. call.

          Show
          Mark Miller added a comment - It's not that it needs to happen before setLeader. The pertinent move it putting it above the super. call.
          Hide
          Mark Miller added a comment - - edited

          Also, the logic here is changed. We now do LIR prevention checks in checkLIR as well. I think it clearly has to come before we declare being the leader. Previously, it was not a checkLIR call - it was logic to just possibly turn LIR off for that replica.

          Show
          Mark Miller added a comment - - edited Also, the logic here is changed. We now do LIR prevention checks in checkLIR as well. I think it clearly has to come before we declare being the leader. Previously, it was not a checkLIR call - it was logic to just possibly turn LIR off for that replica.
          Hide
          Mark Miller added a comment -

          I'll add a little comment above checkLIR: we must check LIR before registering as leader

          Show
          Mark Miller added a comment - I'll add a little comment above checkLIR: we must check LIR before registering as leader
          Hide
          Gregory Chanan added a comment -

          Patch looks good, thanks Mark!

          A comment outside the scope of this JIRA (I know this is pre-existing logic), but which I can't find a better place for:

          +          // We can do this before registering as leader because only setting DOWN requires that
          +          // we are leader, and here we are setting ACTIVE
          +          zkController.updateLeaderInitiatedRecoveryState(collection, shardId,
          +              leaderProps.getStr(ZkStateReader.CORE_NODE_NAME_PROP), Replica.State.ACTIVE, core.getCoreDescriptor(), true);
          

          This seems difficult to reason about given that there are multiple non-commutative writers potentially racing here: a leader setting DOWN and this node setting ACTIVE. It would be easier to reason about if there were two states:
          1) leaders view of the world
          2) replicas view of the world (i.e. telling the Overseer I know the leader thinks I'm in LIR but I know some special information and I'm telling you for this election # I'm OK). That could go in the ZKNodeProps sent to the Overseer (or a separate znode) and the Overseer could do the correct logic with it.

          Anyway, outside of the scope of the jira, just wanted to jot my thoughts down. if you think this is a valid improvement I can file a jira for it.

          Show
          Gregory Chanan added a comment - Patch looks good, thanks Mark! A comment outside the scope of this JIRA (I know this is pre-existing logic), but which I can't find a better place for: + // We can do this before registering as leader because only setting DOWN requires that + // we are leader, and here we are setting ACTIVE + zkController.updateLeaderInitiatedRecoveryState(collection, shardId, + leaderProps.getStr(ZkStateReader.CORE_NODE_NAME_PROP), Replica.State.ACTIVE, core.getCoreDescriptor(), true ); This seems difficult to reason about given that there are multiple non-commutative writers potentially racing here: a leader setting DOWN and this node setting ACTIVE. It would be easier to reason about if there were two states: 1) leaders view of the world 2) replicas view of the world (i.e. telling the Overseer I know the leader thinks I'm in LIR but I know some special information and I'm telling you for this election # I'm OK). That could go in the ZKNodeProps sent to the Overseer (or a separate znode) and the Overseer could do the correct logic with it. Anyway, outside of the scope of the jira, just wanted to jot my thoughts down. if you think this is a valid improvement I can file a jira for it.
          Hide
          Mark Miller added a comment -

          This node has to be the leader in the zk election line at this place it sets active - so there cannot be another valid leader.

          Show
          Mark Miller added a comment - This node has to be the leader in the zk election line at this place it sets active - so there cannot be another valid leader.
          Hide
          Mark Miller added a comment -

          I'd file a JIRA issue to track the idea. It wouldn't be bad to tighten this up.

          Show
          Mark Miller added a comment - I'd file a JIRA issue to track the idea. It wouldn't be bad to tighten this up.
          Hide
          Gregory Chanan added a comment -

          This node has to be the leader in the zk election line at this place it sets active - so there cannot be another valid leader.

          Agreed, my point is just:
          1) that's true now, maybe not always true
          2) that's more to reason about, better to keep things as simple as possible, at least where state is concerned

          I'd file a JIRA issue to track the idea. It wouldn't be bad to tighten this up.

          Will do, thanks.

          Show
          Gregory Chanan added a comment - This node has to be the leader in the zk election line at this place it sets active - so there cannot be another valid leader. Agreed, my point is just: 1) that's true now, maybe not always true 2) that's more to reason about, better to keep things as simple as possible, at least where state is concerned I'd file a JIRA issue to track the idea. It wouldn't be bad to tighten this up. Will do, thanks.
          Hide
          ASF subversion and git services added a comment -

          Commit 1714204 from Mark Miller in branch 'dev/trunk'
          [ https://svn.apache.org/r1714204 ]

          SOLR-8075: Fix faulty implementation.

          Show
          ASF subversion and git services added a comment - Commit 1714204 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1714204 ] SOLR-8075 : Fix faulty implementation.
          Hide
          ASF subversion and git services added a comment -

          Commit 1714205 from Mark Miller in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1714205 ]

          SOLR-8075: Fix faulty implementation.

          Show
          ASF subversion and git services added a comment - Commit 1714205 from Mark Miller in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1714205 ] SOLR-8075 : Fix faulty implementation.
          Hide
          Mark Miller added a comment -

          Note that this needs SOLR-8367 to be complete.

          Show
          Mark Miller added a comment - Note that this needs SOLR-8367 to be complete.

            People

            • Assignee:
              Mark Miller
              Reporter:
              Mark Miller
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development