[GitHub] [lucene-solr] ErickErickson commented on a change in pull request #1756: SOLR-14750: Invoke core reload with core id to avoid multiple reloads

[GitBox <gi...@apache.org>]

ErickErickson commented on a change in pull request #1756:
URL: https://github.com/apache/lucene-solr/pull/1756#discussion_r471489054



##########
File path: solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java
##########
@@ -18,17 +18,7 @@
 
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Locale;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;

Review comment:
       I've had this happen recently with IntelliJ, was this intentional?

##########
File path: solr/core/src/java/org/apache/solr/schema/SchemaManager.java
##########
@@ -21,10 +21,7 @@
 import java.io.StringWriter;
 import java.lang.invoke.MethodHandles;
 import java.nio.charset.StandardCharsets;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;

Review comment:
       I've had this happen recently with IntelliJ, was this intentional?

##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -16,7 +16,6 @@
  */

Review comment:
       This PR doesn't change the failures of TestBulkSchemaConcurrent. The first error I see in a filed run is an NPE at
   at org.apache.solr.core.SolrCore.close(SolrCore.java:1573)
   
         coreMetricManager.close();
   
   which are _not_ thrown for passing iterations. I backed out to just before SOLR-14151 and don't have this problem, whereas as soon as I went from that commit, this fails beasting for me very quickly.
   
   So while this patch seems a reasonable place to start, it doesn't address the failures.

##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1592,93 +1591,94 @@ public void reload(String name, UUID coreId) {
     }
     SolrCore newCore = null;
     SolrCore core = solrCores.getCoreFromAnyList(name, false);

Review comment:
       I think there's still a race condition here. Until we get through waitAddPendingCoreOps, there's no guarantee this core will still be there.
   
   

##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1592,93 +1591,94 @@ public void reload(String name, UUID coreId) {
     }
     SolrCore newCore = null;
     SolrCore core = solrCores.getCoreFromAnyList(name, false);
-    if (core != null) {
-      if(coreId != null && core.uniqueId != coreId) {
-        //trying to reload an already unloaded core
-        return;
+    if (core == null) {
+      CoreLoadFailure clf = coreInitFailures.get(name);
+      if (clf != null) {
+        try {
+          solrCores.waitAddPendingCoreOps(clf.cd.getName());

Review comment:
       I've wondered if this makes sense. I was playing around with moving the waitAddPendingCoreOps(name) to the very first thing in this method and removing this. The only time I think it would be necessary to do this here would be if clf.cd.getName() would ever be different than the name passed in. WDYT?




----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org