Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-5903

SolrCore should implement Closeable

    Details

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

      Description

      Now that we're on Java 7, we can use try-with-resources everywhere we open/close SolrCores.

      1. SOLR-5903.patch
        44 kB
        Alan Woodward

        Issue Links

          Activity

          Hide
          romseygeek Alan Woodward added a comment -

          Patch cutting over most uses of SolrCore.close() to try-with-resources. There are still a few instances I've left alone, mainly in the CoreContainer create-and-register-and-get methods, and in some test classes where changing it would change the logic of the test itself.

          Show
          romseygeek Alan Woodward added a comment - Patch cutting over most uses of SolrCore.close() to try-with-resources. There are still a few instances I've left alone, mainly in the CoreContainer create-and-register-and-get methods, and in some test classes where changing it would change the logic of the test itself.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          Great idea - first thing I noticed on a glance was this part where the success boolean is still defined but not used ... is this right? Seems we will cancel every time?

               boolean success = false;
               try {
          @@ -299,8 +292,8 @@ final class ShardLeaderElectionContext extends ShardLeaderElectionContextBase {
               } catch (Exception e) {
                 SolrException.log(log, "There was a problem trying to register as the leader", e);
             
          -      try {
          -        core = cc.getCore(coreName);
          +      try (SolrCore core = cc.getCore(coreName)) {
          +
                   if (core == null) {
                     throw new SolrException(ErrorCode.SERVER_ERROR,
                         "Fatal Error, SolrCore not found:" + coreName + " in "
          @@ -312,16 +305,7 @@ final class ShardLeaderElectionContext extends ShardLeaderElectionContextBase {
                   // we could not publish ourselves as leader - rejoin election
                   rejoinLeaderElection(leaderSeqPath, core);
                 } finally {
          -        try {
          -          if (!success) {
          -            cancelElection();
          -          }
          -        } finally {
          -          if (core != null) {
          -            core.close();
          -          }
          -        }
          -        
          +        cancelElection();
                 }
               }
          
          Show
          markrmiller@gmail.com Mark Miller added a comment - Great idea - first thing I noticed on a glance was this part where the success boolean is still defined but not used ... is this right? Seems we will cancel every time? boolean success = false; try { @@ -299,8 +292,8 @@ final class ShardLeaderElectionContext extends ShardLeaderElectionContextBase { } catch (Exception e) { SolrException.log(log, "There was a problem trying to register as the leader", e); - try { - core = cc.getCore(coreName); + try (SolrCore core = cc.getCore(coreName)) { + if (core == null) { throw new SolrException(ErrorCode.SERVER_ERROR, "Fatal Error, SolrCore not found:" + coreName + " in " @@ -312,16 +305,7 @@ final class ShardLeaderElectionContext extends ShardLeaderElectionContextBase { // we could not publish ourselves as leader - rejoin election rejoinLeaderElection(leaderSeqPath, core); } finally { - try { - if (!success) { - cancelElection(); - } - } finally { - if (core != null) { - core.close(); - } - } - + cancelElection(); } }
          Hide
          romseygeek Alan Woodward added a comment -

          That threw me the first time I saw it as well - it's because this particular try-finally is nested within a catch block, so success is always false.

          Maybe instead success should be set to true after the call to rejoinLeaderElection()? I'm not familiar with how the leader election semantics work.

          Show
          romseygeek Alan Woodward added a comment - That threw me the first time I saw it as well - it's because this particular try-finally is nested within a catch block, so success is always false. Maybe instead success should be set to true after the call to rejoinLeaderElection()? I'm not familiar with how the leader election semantics work.
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          Yeah, I see - that's already a little jacked up. I'll fix it.

          Show
          markrmiller@gmail.com Mark Miller added a comment - Yeah, I see - that's already a little jacked up. I'll fix it.
          Hide
          romseygeek Alan Woodward added a comment -

          Do you want to wait for SOLR-5904, or should I just commit this now?

          Show
          romseygeek Alan Woodward added a comment - Do you want to wait for SOLR-5904 , or should I just commit this now?
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          No, fire away, I'll merge things in for SOLR-5904.

          Show
          markrmiller@gmail.com Mark Miller added a comment - No, fire away, I'll merge things in for SOLR-5904 .
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1581360 from Alan Woodward in branch 'dev/trunk'
          [ https://svn.apache.org/r1581360 ]

          SOLR-5903: SolrCore implements Closeable

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1581360 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1581360 ] SOLR-5903 : SolrCore implements Closeable
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1581363 from Alan Woodward in branch 'dev/branches/branch_4x'
          [ https://svn.apache.org/r1581363 ]

          SOLR-5903: SolrCore implements Closeable

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1581363 from Alan Woodward in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1581363 ] SOLR-5903 : SolrCore implements Closeable
          Hide
          thetaphi Uwe Schindler added a comment -

          Close issue after release of 4.8.0

          Show
          thetaphi Uwe Schindler added a comment - Close issue after release of 4.8.0

            People

            • Assignee:
              romseygeek Alan Woodward
              Reporter:
              romseygeek Alan Woodward
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development