patsonluk commented on code in PR #2607:
URL: https://github.com/apache/solr/pull/2607#discussion_r1698868734


##########
solr/core/src/java/org/apache/solr/core/SyntheticSolrCore.java:
##########
@@ -72,4 +88,43 @@ protected RestManager initRestManager() throws SolrException 
{
     // We do not expect RestManager ops on Coordinator Nodes
     return new RestManager();
   }
+
+  @Override
+  public SolrCore reload(ConfigSet coreConfig) throws IOException {
+    // only one reload at a time
+    synchronized (getUpdateHandler().getSolrCoreState().getReloadLock()) {
+      solrCoreState.increfSolrCoreState();
+      boolean success = false;
+      SyntheticSolrCore newCore = null;
+      try {
+        CoreDescriptor newCoreDescriptor = new CoreDescriptor(getName(), 
getCoreDescriptor());
+        newCoreDescriptor.loadExtraProperties(); // Reload the extra properties
+
+        newCore =
+            new SyntheticSolrCore(
+                getCoreContainer(),
+                newCoreDescriptor,
+                coreConfig,
+                getDataDir(),
+                getUpdateHandler(),
+                getDeletionPolicy(),
+                this,
+                true);
+
+        newCore.getSearcher(true, false, null, true);
+        success = true;
+        return newCore;
+      } finally {
+        // close the new core on any errors that have occurred.
+        if (!success && newCore != null && newCore.getOpenCount() > 0) {

Review Comment:
   This part is copied from `SolrCore#reload`, I don't know the code super well 
so I'm just making guesses here 😅  - This makes sure whatever resource 
allocated by a "failed" core to be released before leaving this method. I think 
for normal successful case, it should not close the new core returned.
   
   `Maybe it's better to switch them directly instead of returning and jumping 
back to close?` are you referring to the fact that a non null core can be 
returned yet it's getting closed here if it's a bad one? Honestly I don't 
understand the full rational behind the comment `// close the new core on any 
errors that have occurred.` neither 🤔 . Though it's probably the safest to just 
follow suit in case this is needed to prevent some unexpected resource leak 
perhaps.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org

Reply via email to