Solr
  1. Solr
  2. SOLR-8288

DistributedUpdateProcessor#doFinish should explicitly check and ensure it does not try to put itself into LIR.

    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

      We have to be careful about this because currently, something like a commit is sent over http even to the local node and if that fails for some reason, the leader might try and LIR itself.

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          First patch.

          Show
          Mark Miller added a comment - First patch.
          Hide
          Mike Drob added a comment -

          + if (leaderCoreNodeName != null && cloudDesc.getCoreNodeName().equals(leaderCoreNodeName) // we are still same leader

          This null check is not necessary.

          Is it worth adding a test where a node tries to put itself into recovery? Not sure if that's something that is actually possible to stub out.

          Show
          Mike Drob added a comment - + if (leaderCoreNodeName != null && cloudDesc.getCoreNodeName().equals(leaderCoreNodeName) // we are still same leader This null check is not necessary. Is it worth adding a test where a node tries to put itself into recovery? Not sure if that's something that is actually possible to stub out.
          Hide
          Mark Miller added a comment -

          This null check is not necessary.

          I was originally trying to use leaderCoreNodeName for the identity check, in which case you do need the null check. I switched to using the core url for the check, but I have left this null check in - I find it makes it much more explicit that that variable can be null here, rather than just counting on the fact that the equals method will handle the null how we want.

          Is it worth adding a test where a node tries to put itself into recovery?

          If you can add a test for this, it would be nice to have, but I don't see a good way to do it without some invasive ugly code. It should probably spin out into it's own JIRA unless something can be done quickly.

          Show
          Mark Miller added a comment - This null check is not necessary. I was originally trying to use leaderCoreNodeName for the identity check, in which case you do need the null check. I switched to using the core url for the check, but I have left this null check in - I find it makes it much more explicit that that variable can be null here, rather than just counting on the fact that the equals method will handle the null how we want. Is it worth adding a test where a node tries to put itself into recovery? If you can add a test for this, it would be nice to have, but I don't see a good way to do it without some invasive ugly code. It should probably spin out into it's own JIRA unless something can be done quickly.
          Hide
          Mike Drob added a comment -

          If you can add a test for this, it would be nice to have, but I don't see a good way to do it without some invasive ugly code. It should probably spin out into it's own JIRA unless something can be done quickly.

          Yea, I don't see an immediately apparent way to do this. Was hoping you knew something I didn't. Filed SOLR-8289.

          Show
          Mike Drob added a comment - If you can add a test for this, it would be nice to have, but I don't see a good way to do it without some invasive ugly code. It should probably spin out into it's own JIRA unless something can be done quickly. Yea, I don't see an immediately apparent way to do this. Was hoping you knew something I didn't. Filed SOLR-8289 .
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-8288: DistributedUpdateProcessor#doFinish should explicitly check and ensure it does not try to put itself into LIR.

          Show
          ASF subversion and git services added a comment - Commit 1714271 from Mark Miller in branch 'dev/trunk' [ https://svn.apache.org/r1714271 ] SOLR-8288 : DistributedUpdateProcessor#doFinish should explicitly check and ensure it does not try to put itself into LIR.
          Hide
          ASF subversion and git services added a comment -

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

          SOLR-8288: DistributedUpdateProcessor#doFinish should explicitly check and ensure it does not try to put itself into LIR.

          Show
          ASF subversion and git services added a comment - Commit 1714272 from Mark Miller in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1714272 ] SOLR-8288 : DistributedUpdateProcessor#doFinish should explicitly check and ensure it does not try to put itself into LIR.
          Hide
          Mike Drob added a comment -

          + && !stdNode.getNodeProps().getCoreUrl().equals(leaderProps.getCoreUrl())) { // we do not want to put ourself into LIR

          If we are comparing URLs, then would it make sense to check against the replicaURL that we saw the error on?

          replicaUrl.equals(leaderProps.getCoreUrl()) instead?

          Show
          Mike Drob added a comment - + && !stdNode.getNodeProps().getCoreUrl().equals(leaderProps.getCoreUrl())) { // we do not want to put ourself into LIR If we are comparing URLs, then would it make sense to check against the replicaURL that we saw the error on? replicaUrl.equals(leaderProps.getCoreUrl()) instead?

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development