dsmiley commented on code in PR #1243:
URL: https://github.com/apache/solr/pull/1243#discussion_r1056032867
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -1772,11 +1786,16 @@ private void resetIndexDirectory(CoreDescriptor dcore,
ConfigSet coreConfig) {
}
/**
- * Gets the permanent (non-transient) cores that are currently loaded.
+ * Gets all loaded cores, consistent with {@link #getLoadedCoreNames()}.
Caller doesn't need to
+ * close.
+ *
+ * <p>NOTE: rather dangerous API because each core is not reserved (could in
theory be closed).
+ * Prefer {@link #getLoadedCoreNames()} and then call {@link
#getCore(String)} then close it.
*
* @return An unsorted list. This list is a new copy, it can be modified by
the caller (e.g. it
- * can be sorted).
+ * can be sorted). Don't need to close them.
*/
+ @Deprecated
Review Comment:
Deprecated because it's trappy -- cores are not incremented; perhaps they
might be closed in the middle of a caller's processing. It's not hard for
callers to return a list of core names and fetch each calling close instead. I
didn't change most callers yet though.
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2284,17 +2303,6 @@ public boolean isLoaded(String name) {
return solrCores.isLoaded(name);
}
- /**
- * Gets a solr core descriptor for a core that is not loaded. Note that if
the caller calls this
- * on a loaded core, the unloaded descriptor will be returned.
- *
- * @param cname - name of the unloaded core descriptor to load. NOTE:
- * @return a coreDescriptor. May return null
- */
- public CoreDescriptor getUnloadedCoreDescriptor(String cname) {
Review Comment:
wasn't useful
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -36,60 +36,46 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-class SolrCores {
+/** Holds/manages {@link SolrCore}s within {@link CoreContainer}. */
+public class SolrCores {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ // for locking around manipulating any of the core maps.
+ protected final Object modifyLock = new Object();
- private static final Object modifyLock =
- new Object(); // for locking around manipulating any of the core maps.
private final Map<String, SolrCore> cores = new LinkedHashMap<>(); // For
"permanent" cores
// These descriptors, once loaded, will _not_ be unloaded, i.e. they are not
"transient".
private final Map<String, CoreDescriptor> residentDescriptors = new
LinkedHashMap<>();
private final CoreContainer container;
- private Set<String> currentlyLoadingCores =
+ private final Set<String> currentlyLoadingCores =
Collections.newSetFromMap(new ConcurrentHashMap<String, Boolean>());
- private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
-
// This map will hold objects that are being currently operated on. The core
(value) may be null
// in the case of initial load. The rule is, never to any operation on a
core that is currently
// being operated upon.
- private static final Set<String> pendingCoreOps = new HashSet<>();
+ private final Set<String> pendingCoreOps = new HashSet<>();
// Due to the fact that closes happen potentially whenever anything is
_added_ to the transient
// core list, we need to essentially queue them up to be handled via
pendingCoreOps.
- private static final List<SolrCore> pendingCloses = new ArrayList<>();
Review Comment:
a couple fields in here like modifyLock and this were static which made no
sense so I removed that.
##########
solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java:
##########
@@ -379,8 +378,13 @@ private static NodeConfig
fillSolrSection(NodeConfig.NodeConfigBuilder builder,
case "replayUpdatesThreads":
builder.setReplayUpdatesThreads(it.intVal(-1));
break;
- case "transientCacheSize":
- builder.setTransientCacheSize(it.intVal(-1));
+ case "transientCacheSize": // TODO declare deprecated
Review Comment:
TODO is to remember to state this in the release notes
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -99,37 +85,34 @@ protected void close() {
waitForLoadingCoresToFinish(30 * 1000);
Collection<SolrCore> coreList = new ArrayList<>();
- // Release transient core cache.
- synchronized (modifyLock) {
- if (transientSolrCoreCacheFactory != null) {
- getTransientCacheHandler().close();
- }
- }
-
// It might be possible for one of the cores to move from one list to
another while we're
// closing them. So loop through the lists until they're all empty. In
particular, the core
// could have moved from the transient list to the pendingCloses list.
- do {
- coreList.clear();
+ while (true) {
+
synchronized (modifyLock) {
- // make a copy of the cores then clear the map so the core isn't
handed out to a request
- // again
Review Comment:
This comment is wrong, I think. Clearing this.cores does not clear the
descriptors; a request coming in will lazy load it. This is finicky stuff so I
opted to simply remove this comment instead of try to fix a possible existing
bug that I've never seen.
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -99,37 +85,34 @@ protected void close() {
waitForLoadingCoresToFinish(30 * 1000);
Collection<SolrCore> coreList = new ArrayList<>();
- // Release transient core cache.
- synchronized (modifyLock) {
- if (transientSolrCoreCacheFactory != null) {
- getTransientCacheHandler().close();
- }
- }
-
// It might be possible for one of the cores to move from one list to
another while we're
// closing them. So loop through the lists until they're all empty. In
particular, the core
// could have moved from the transient list to the pendingCloses list.
- do {
- coreList.clear();
+ while (true) {
+
synchronized (modifyLock) {
- // make a copy of the cores then clear the map so the core isn't
handed out to a request
- // again
- coreList.addAll(cores.values());
- cores.clear();
- if (transientSolrCoreCacheFactory != null) {
- coreList.addAll(getTransientCacheHandler().prepareForShutdown());
+ // remove all loaded cores; add to our working list.
+ for (String name : getLoadedCoreNames()) {
+ final var core = remove(name);
+ if (core != null) {
+ coreList.add(core);
+ }
}
coreList.addAll(pendingCloses);
pendingCloses.clear();
}
+ if (coreList.isEmpty()) {
Review Comment:
I changed the loop to exit at this point, which is more efficient and I
think equally clear.
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -99,37 +85,34 @@ protected void close() {
waitForLoadingCoresToFinish(30 * 1000);
Collection<SolrCore> coreList = new ArrayList<>();
- // Release transient core cache.
- synchronized (modifyLock) {
- if (transientSolrCoreCacheFactory != null) {
- getTransientCacheHandler().close();
- }
- }
-
// It might be possible for one of the cores to move from one list to
another while we're
// closing them. So loop through the lists until they're all empty. In
particular, the core
// could have moved from the transient list to the pendingCloses list.
- do {
- coreList.clear();
+ while (true) {
+
synchronized (modifyLock) {
- // make a copy of the cores then clear the map so the core isn't
handed out to a request
- // again
- coreList.addAll(cores.values());
- cores.clear();
- if (transientSolrCoreCacheFactory != null) {
- coreList.addAll(getTransientCacheHandler().prepareForShutdown());
+ // remove all loaded cores; add to our working list.
+ for (String name : getLoadedCoreNames()) {
+ final var core = remove(name);
+ if (core != null) {
+ coreList.add(core);
+ }
}
coreList.addAll(pendingCloses);
pendingCloses.clear();
}
+ if (coreList.isEmpty()) {
+ break;
+ }
+
ExecutorService coreCloseExecutor =
ExecutorUtil.newMDCAwareFixedThreadPool(
Integer.MAX_VALUE, new
SolrNamedThreadFactory("coreCloseExecutor"));
try {
for (SolrCore core : coreList) {
- coreCloseExecutor.submit(
+ coreCloseExecutor.execute(
Review Comment:
submit() returns a Future and we clearly weren't using it.
Inexplicably, this improvement appears to cause SolrMetricsIntegrationTest
to fail because it double-closed cores. I looked and made that test not do
that. I'm not sure why it worked before. Maybe some fluke/accidental
circumstance.
--
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
