dsmiley commented on code in PR #4764:
URL: https://github.com/apache/solr/pull/4764#discussion_r3817762758
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -1382,13 +1382,12 @@ public void shutdown() {
}
public void cancelCoreRecoveries() {
-
- List<SolrCore> cores = solrCores.getCores();
Review Comment:
Arguably, SolrCores.getCores() shouldn't be deprecated because it's hidden
one layer deep on a class only used by CoreContainer.
##########
solr/core/src/java/org/apache/solr/handler/admin/api/NodeHealth.java:
##########
Review Comment:
nice; this is safer as we avoid race condition on a core closing with
inspecting its health
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -142,20 +142,6 @@ public SolrCore putCore(CoreDescriptor cd, SolrCore core) {
}
}
- /**
- * @return A list of "permanent" cores, i.e. cores that may not be swapped
out and are currently
- * loaded.
- * <p>A core may be non-transient but still lazily loaded. If it is
"permanent" and lazy-load
- * _and_ not yet loaded it will _not_ be returned by this call.
- * <p>This list is a new copy, it can be modified by the caller (e.g. it
can be sorted).
- */
- @Deprecated
Review Comment:
maybe shouldn't be deprecated after all (as I say above
##########
solr/core/src/java/org/apache/solr/pkg/SolrPackageLoader.java:
##########
Review Comment:
again; added safety :-)
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -1382,13 +1382,12 @@ public void shutdown() {
}
public void cancelCoreRecoveries() {
-
- List<SolrCore> cores = solrCores.getCores();
-
// we must cancel without holding the cores sync
// make sure we wait for any recoveries to stop
- for (SolrCore core : cores) {
- try {
+ for (String coreName : solrCores.getLoadedCoreNames()) {
+ // getCoreFromAnyList, not getCore: never loads, safe during shutdown
+ try (SolrCore core = solrCores.getCoreFromAnyList(coreName, true)) {
Review Comment:
how could this be correct, using try-with-resources on a getCore that has
*not* been inc-ref'ed, and thus we shouldn't close it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
