Solr
  1. Solr
  2. SOLR-5903

SolrCore should implement Closeable

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major 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
          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
          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
          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
          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
          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
          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
          Mark Miller added a comment -

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

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

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

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

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

          Show
          Mark Miller added a comment - No, fire away, I'll merge things in for SOLR-5904 .
          Hide
          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
          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
          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
          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
          Uwe Schindler added a comment -

          Close issue after release of 4.8.0

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development