You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/11/06 17:41:14 UTC
[GitHub] [lucene-solr] bruno-roustant opened a new pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
bruno-roustant opened a new pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520029778
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -51,7 +51,7 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.NO_OP;
Review comment:
Under what circumstance do we need this no-op impl to prevent an NPE?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -62,55 +51,44 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientCoreCache;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.NO_OP;
- private TransientSolrCoreCache transientSolrCoreCache = null;
-
SolrCores(CoreContainer container) {
this.container = container;
}
protected void addCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
- } else {
- log.warn("We encountered a core marked as transient, but there is no transient handler defined. This core will be inaccessible");
- }
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
} else {
- residentDesciptors.put(p.getName(), p);
+ residentDescriptors.put(p.getName(), p);
}
}
}
protected void removeCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().removeTransientDescriptor(p.getName());
- }
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
} else {
- residentDesciptors.remove(p.getName());
+ residentDescriptors.remove(p.getName());
}
}
}
public void load(SolrResourceLoader loader) {
- transientCoreCache = TransientSolrCoreCacheFactory.newInstance(loader, container);
+ transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.newInstance(loader, container);
}
+
// We are shutting down. You can't hold the lock on the various lists of cores while they shut down, so we need to
// make a temporary copy of the names and shut them down outside the lock.
protected void close() {
waitForLoadingCoresToFinish(30*1000);
Collection<SolrCore> coreList = new ArrayList<>();
-
- TransientSolrCoreCache transientSolrCoreCache = getTransientCacheHandler();
- // Release observer
- if (transientSolrCoreCache != null) {
- transientSolrCoreCache.close();
- }
+ // Release transient core cache.
+ getTransientCacheHandler().close();
Review comment:
@bruno-roustant the muse bot makes a good point; there should be a synchronized(modifyLock) around grabbing getTransientCacheHandler and calling close on 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.
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519684814
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -490,21 +492,19 @@ public CoreDescriptor getCoreDescriptor(String coreName) {
}
/**
- * Get the CoreDescriptors for every SolrCore managed here
- * @return a List of CoreDescriptors
+ * Get the CoreDescriptors for every {@link SolrCore} managed here (permanent and transient, loaded and unloaded).
+ *
+ * @return An unordered list copy. This list can be modified by the caller (e.g. sorted).
Review comment:
I'll use ArrayList.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520057072
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -51,7 +51,7 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.NO_OP;
Review comment:
then SolrCores.load could be called much sooner, basically right after the resourceLoader is ready.
Also, maybe this other thread ought to wait to start till some later time. Perhaps ideally there would be an event publishing mechanism, which doesn't exist currently, I know. Or alternatively just have some CountDownLatch signal, like signaling when Solr will begin loading cores.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723503967
I'll continue soon with two other improvements:
- Optimize SolrCores.getCoreDescriptors().
- NOOP TransientSolrCoreCache.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519224133
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1568,33 +1568,49 @@ private void resetIndexDirectory(CoreDescriptor dcore, ConfigSet coreConfig) {
}
/**
- * @return a Collection of registered SolrCores
+ * Gets the permanent (non-transient) cores that are currently loaded.
+ *
+ * @return An unsorted collection.
*/
public Collection<SolrCore> getCores() {
Review comment:
Very good point!
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520049002
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -62,55 +51,44 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientCoreCache;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.NO_OP;
- private TransientSolrCoreCache transientSolrCoreCache = null;
-
SolrCores(CoreContainer container) {
this.container = container;
}
protected void addCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
- } else {
- log.warn("We encountered a core marked as transient, but there is no transient handler defined. This core will be inaccessible");
- }
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
} else {
- residentDesciptors.put(p.getName(), p);
+ residentDescriptors.put(p.getName(), p);
}
}
}
protected void removeCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().removeTransientDescriptor(p.getName());
- }
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
} else {
- residentDesciptors.remove(p.getName());
+ residentDescriptors.remove(p.getName());
}
}
}
public void load(SolrResourceLoader loader) {
- transientCoreCache = TransientSolrCoreCacheFactory.newInstance(loader, container);
+ transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.newInstance(loader, container);
}
+
// We are shutting down. You can't hold the lock on the various lists of cores while they shut down, so we need to
// make a temporary copy of the names and shut them down outside the lock.
protected void close() {
waitForLoadingCoresToFinish(30*1000);
Collection<SolrCore> coreList = new ArrayList<>();
-
- TransientSolrCoreCache transientSolrCoreCache = getTransientCacheHandler();
- // Release observer
- if (transientSolrCoreCache != null) {
- transientSolrCoreCache.close();
- }
+ // Release transient core cache.
+ getTransientCacheHandler().close();
Review comment:
+1 thanks muse!
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723587255
So @dsmiley @ErickErickson do we all agree that we throw an exception in in TransientSolrCoreCacheFactory.newInstance() line 60 and we never return null?
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519227113
##########
File path: solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java
##########
@@ -69,7 +71,10 @@ public void startReplication(boolean switchTransactionLog) {
if (cc.isShutDown()) {
return;
} else {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "SolrCore not found:" + coreName + " in " + cc.getLoadedCoreNames());
+ Collection<String> loadedCoreNames = cc.getLoadedCoreNames();
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "SolrCore not found:" + coreName + " in "
Review comment:
Ok. I'll share in a new method CloudUtil.getLoadedCoreNamesAsString().
----------------------------------------------------------------
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
[GitHub] [lucene-solr] ErickErickson commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723577563
David:
I’ve no objection to removing all the if (transientHandler==null) tests if we fail on initialization.
Bruno:
Yeah, I think 9.0 only is the way to go.
> On Nov 8, 2020, at 1:35 AM, David Smiley <no...@github.com> wrote:
>
>
> @dsmiley commented on this pull request.
>
> In solr/core/src/java/org/apache/solr/core/SolrCores.java:
>
> > synchronized (modifyLock) {
> - set = new TreeSet<>(cores.keySet());
> - if (getTransientCacheHandler() != null) {
> - set.addAll(getTransientCacheHandler().getLoadedCoreNames());
> - }
> + TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
>
> @ErickErickson IMO a bad config should stop Solr in its tracks. Why allow this? I'd rather disallow it and thus we'd always have either the default impl or some custom impl, but something.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
>
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520058820
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -536,7 +538,9 @@ public void queueCoreToClose(SolrCore coreToClose) {
* @return the cache holding the transient cores; never null.
*/
public TransientSolrCoreCache getTransientCacheHandler() {
Review comment:
I think it should be a prerequisite that the caller acquire the lock a-priori. For example, close() seems to need to keep holding this lock to call close() on 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.
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
[GitHub] [lucene-solr] bruno-roustant commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723586994
Finally I optimized SolrCore.getCoreDescriptors(). The version in this commit is the one for the 8.x branch - TransientSolrCoreCache.getTransientDescriptors() stays compatible with a default (sub-optimal but still better) implementation overridden by the optimal implementation in TransientSolrCoreCacheDefault.
When I merge this PR, I'll simply remove the default impl on master.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520089739
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -536,7 +538,9 @@ public void queueCoreToClose(SolrCore coreToClose) {
* @return the cache holding the transient cores; never null.
*/
public TransientSolrCoreCache getTransientCacheHandler() {
Review comment:
I don't think so. Let's say transientSolrCoreCacheFactory = A. If there is a race and load() is called between getTransientCacheHandler() and TransientSolrCoreCache.close(). getTransientCacheHandler() returns A, then load() sets B, and then A.close() is called. It is the same result as the sequence { getTransientCacheHandler().close() } atomically on A, and then load() B.
But actually it doesn't matter. So I can add a synchronized (modifyLock) around if you prefer.
I'll keep anyway the synchronized inside getTransientCacheHandler() because it is public.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519473036
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -490,21 +492,19 @@ public CoreDescriptor getCoreDescriptor(String coreName) {
}
/**
- * Get the CoreDescriptors for every SolrCore managed here
- * @return a List of CoreDescriptors
+ * Get the CoreDescriptors for every {@link SolrCore} managed here (permanent and transient, loaded and unloaded).
+ *
+ * @return An unordered list copy. This list can be modified by the caller (e.g. sorted).
Review comment:
Collectors.toList says that there is no guarantees on the mutability. So remove the docs about the mutability guarantee of what we return because we can't guarantee that.
Or, simply use ArrayList and call addAll. I prefer this; use of Streams here doesn't appear to add value.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519224315
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -198,35 +188,91 @@ protected SolrCore putCore(CoreDescriptor cd, SolrCore core) {
*
* Put another way, this will not return any names of cores that are lazily loaded but have not been called for yet
* or are transient and either not loaded or have been swapped out.
- *
- * @return List of currently loaded cores.
+ *
+ * @return A unsorted collection.
*/
- Set<String> getLoadedCoreNames() {
- Set<String> set;
-
+ Collection<String> getLoadedCoreNames() {
synchronized (modifyLock) {
- set = new TreeSet<>(cores.keySet());
- if (getTransientCacheHandler() != null) {
- set.addAll(getTransientCacheHandler().getLoadedCoreNames());
- }
+ TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
+ Set<String> transientCoreNames = transientCoreCache == null ? Collections.emptySet() : transientCoreCache.getLoadedCoreNames();
+ return distinctSetsUnion(cores.keySet(), transientCoreNames);
}
- return set;
}
+
/**
- * Gets a list of all cores, loaded and unloaded
+ * Gets a collection of all cores names, loaded and unloaded.
+ * For efficiency, prefer to check {@link #getCoreDescriptor(String)} != null instead of {@link #getAllCoreNames()}.contains(String)
*
- * @return all cores names, whether loaded or unloaded, transient or permanent.
+ * @return A unsorted collection.
*/
public Collection<String> getAllCoreNames() {
- Set<String> set;
synchronized (modifyLock) {
- set = new TreeSet<>(cores.keySet());
- if (getTransientCacheHandler() != null) {
- set.addAll(getTransientCacheHandler().getAllCoreNames());
- }
- set.addAll(residentDesciptors.keySet());
+ TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
+ Set<String> transientCoreNames = transientCoreCache == null ? Collections.emptySet() : transientCoreCache.getAllCoreNames();
+ return distinctSetsUnion(residentDescriptors.keySet(), transientCoreNames);
+ }
+ }
+
+ /**
+ * Makes the union of two distinct sets.
+ */
+ private static <T> Collection<T> distinctSetsUnion(Set<T> set1, Set<T> set2) {
+ assert areSetsDistinct(set1, set2);
Review comment:
Yes it must be a snapshot (like in getCores()). I'll add the comment.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r518937449
##########
File path: solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java
##########
@@ -69,7 +71,10 @@ public void startReplication(boolean switchTransactionLog) {
if (cc.isShutDown()) {
return;
} else {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "SolrCore not found:" + coreName + " in " + cc.getLoadedCoreNames());
+ Collection<String> loadedCoreNames = cc.getLoadedCoreNames();
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "SolrCore not found:" + coreName + " in "
Review comment:
Nice!
##########
File path: solr/core/src/java/org/apache/solr/cloud/ReplicateFromLeader.java
##########
@@ -69,7 +71,10 @@ public void startReplication(boolean switchTransactionLog) {
if (cc.isShutDown()) {
return;
} else {
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "SolrCore not found:" + coreName + " in " + cc.getLoadedCoreNames());
+ Collection<String> loadedCoreNames = cc.getLoadedCoreNames();
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "SolrCore not found:" + coreName + " in "
Review comment:
If there are fewer than 20, list them sorted? Since this error/log construction logic exists in two places, maybe put this in one spot.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -198,35 +188,91 @@ protected SolrCore putCore(CoreDescriptor cd, SolrCore core) {
*
* Put another way, this will not return any names of cores that are lazily loaded but have not been called for yet
* or are transient and either not loaded or have been swapped out.
- *
- * @return List of currently loaded cores.
+ *
+ * @return A unsorted collection.
*/
- Set<String> getLoadedCoreNames() {
- Set<String> set;
-
+ Collection<String> getLoadedCoreNames() {
synchronized (modifyLock) {
- set = new TreeSet<>(cores.keySet());
- if (getTransientCacheHandler() != null) {
- set.addAll(getTransientCacheHandler().getLoadedCoreNames());
- }
+ TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
+ Set<String> transientCoreNames = transientCoreCache == null ? Collections.emptySet() : transientCoreCache.getLoadedCoreNames();
+ return distinctSetsUnion(cores.keySet(), transientCoreNames);
}
- return set;
}
+
/**
- * Gets a list of all cores, loaded and unloaded
+ * Gets a collection of all cores names, loaded and unloaded.
+ * For efficiency, prefer to check {@link #getCoreDescriptor(String)} != null instead of {@link #getAllCoreNames()}.contains(String)
*
- * @return all cores names, whether loaded or unloaded, transient or permanent.
+ * @return A unsorted collection.
*/
public Collection<String> getAllCoreNames() {
- Set<String> set;
synchronized (modifyLock) {
- set = new TreeSet<>(cores.keySet());
- if (getTransientCacheHandler() != null) {
- set.addAll(getTransientCacheHandler().getAllCoreNames());
- }
- set.addAll(residentDesciptors.keySet());
+ TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
+ Set<String> transientCoreNames = transientCoreCache == null ? Collections.emptySet() : transientCoreCache.getAllCoreNames();
+ return distinctSetsUnion(residentDescriptors.keySet(), transientCoreNames);
+ }
+ }
+
+ /**
+ * Makes the union of two distinct sets.
+ */
+ private static <T> Collection<T> distinctSetsUnion(Set<T> set1, Set<T> set2) {
+ assert areSetsDistinct(set1, set2);
Review comment:
Further comment that this guarantees to return a new copy, as it must do so because the caller needs to return a thread-safe snapshot. Before I recalled that, I was going to suggest returning set1 or set2 if the opposite is empty, but we can't.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -198,35 +188,91 @@ protected SolrCore putCore(CoreDescriptor cd, SolrCore core) {
*
* Put another way, this will not return any names of cores that are lazily loaded but have not been called for yet
* or are transient and either not loaded or have been swapped out.
- *
- * @return List of currently loaded cores.
+ *
+ * @return A unsorted collection.
*/
- Set<String> getLoadedCoreNames() {
- Set<String> set;
-
+ Collection<String> getLoadedCoreNames() {
synchronized (modifyLock) {
- set = new TreeSet<>(cores.keySet());
- if (getTransientCacheHandler() != null) {
- set.addAll(getTransientCacheHandler().getLoadedCoreNames());
- }
+ TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
Review comment:
There is always a TransientSolrCoreCache, even if it's not used. Right? CC @ErickErickson
So why all these null checks if I'm right? If I'm wrong, I suspect it'd be cleaner to have a no-op impl so that the code around here is easier on the eyes, because there are _many_ null checks.
##########
File path: solr/core/src/java/org/apache/solr/core/ZkContainer.java
##########
@@ -125,14 +124,8 @@ public void initZooKeeper(final CoreContainer cc, CloudConfig config) {
"A chroot was specified in ZkHost but the znode doesn't exist. " + zookeeperHost);
}
- Supplier<List<CoreDescriptor>> descriptorsSupplier = () -> {
- List<CoreDescriptor> descriptors = new ArrayList<>(cc.getLoadedCoreNames().size());
- Collection<SolrCore> cores = cc.getCores();
- for (SolrCore core : cores) {
- descriptors.add(core.getCoreDescriptor());
- }
- return descriptors;
- };
+ Supplier<List<CoreDescriptor>> descriptorsSupplier = () ->
+ cc.getCores().stream().map(SolrCore::getCoreDescriptor).collect(Collectors.toList());
Review comment:
beautiful application of streams
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1568,33 +1568,49 @@ private void resetIndexDirectory(CoreDescriptor dcore, ConfigSet coreConfig) {
}
/**
- * @return a Collection of registered SolrCores
+ * Gets the permanent (non-transient) cores that are currently loaded.
+ *
+ * @return An unsorted collection.
*/
public Collection<SolrCore> getCores() {
Review comment:
Thanks for the javadoc. I think this is method should be returning all loaded cores, but is not. The callers (excluding the ones you changed to _not_ call it any more) I think want loaded cores. They don't know that they are not getting the transient ones, and furthermore extremely few Solr users even use transient cores, so this simply isn't noticed. I shall file an issue about this because fixing that would be out of scope here.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519263690
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -198,35 +188,91 @@ protected SolrCore putCore(CoreDescriptor cd, SolrCore core) {
*
* Put another way, this will not return any names of cores that are lazily loaded but have not been called for yet
* or are transient and either not loaded or have been swapped out.
- *
- * @return List of currently loaded cores.
+ *
+ * @return A unsorted collection.
*/
- Set<String> getLoadedCoreNames() {
- Set<String> set;
-
+ Collection<String> getLoadedCoreNames() {
synchronized (modifyLock) {
- set = new TreeSet<>(cores.keySet());
- if (getTransientCacheHandler() != null) {
- set.addAll(getTransientCacheHandler().getLoadedCoreNames());
- }
+ TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
Review comment:
@ErickErickson IMO a bad config should stop Solr in its tracks. Why allow this? I'd rather disallow it and thus we'd *always* have either the default impl or some custom impl, but _something_.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723661590
> So @dsmiley @ErickErickson do we all agree that we throw an exception in in TransientSolrCoreCacheFactory.newInstance() line 60 and we never return null?
+1 definitely
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519792336
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -121,8 +101,8 @@ protected void close() {
// 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 (transientSolrCoreCache != null) {
- coreList.addAll(transientSolrCoreCache.prepareForShutdown());
+ if (transientSolrCoreCacheFactory != null) {
Review comment:
Do we still need a null check here? And why the factory vs the cache itself?
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant closed pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant closed pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520092229
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -536,7 +538,9 @@ public void queueCoreToClose(SolrCore coreToClose) {
* @return the cache holding the transient cores; never null.
*/
public TransientSolrCoreCache getTransientCacheHandler() {
Review comment:
Okay; I'm fine with the lock to be safe.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723568609
I'm looking at optimizing SolrCores.getCoreDescriptors() but actually it would require to add a new TransientSolrCoreCache.getTransientDescriptors() method. This means the public interface TransientSolrCoreCache would not be compatible. I suppose we shouldn't do that in this PR but rather in another for 9.x only? @dsmiley
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520040508
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -51,7 +51,7 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.NO_OP;
Review comment:
If a SolrCores method accessing getTransientCacheHandler() is called before calling load().
For example an asynchronous periodic thread that would be started with CoreContainer and that would call getLoadedCoreNames(). This worked before even if SolrCores.load() was called after. (first calls would return without counting transient cores, and subsequent calls after load() would count transient cores)
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723960636
Ok, now I think I integrated all comments.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] muse-dev[bot] commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519994863
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -62,55 +51,44 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientCoreCache;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.NO_OP;
- private TransientSolrCoreCache transientSolrCoreCache = null;
-
SolrCores(CoreContainer container) {
this.container = container;
}
protected void addCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
- } else {
- log.warn("We encountered a core marked as transient, but there is no transient handler defined. This core will be inaccessible");
- }
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
} else {
- residentDesciptors.put(p.getName(), p);
+ residentDescriptors.put(p.getName(), p);
}
}
}
protected void removeCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().removeTransientDescriptor(p.getName());
- }
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
} else {
- residentDesciptors.remove(p.getName());
+ residentDescriptors.remove(p.getName());
}
}
}
public void load(SolrResourceLoader loader) {
- transientCoreCache = TransientSolrCoreCacheFactory.newInstance(loader, container);
+ transientSolrCoreCacheFactory = TransientSolrCoreCacheFactory.newInstance(loader, container);
}
+
// We are shutting down. You can't hold the lock on the various lists of cores while they shut down, so we need to
// make a temporary copy of the names and shut them down outside the lock.
protected void close() {
waitForLoadingCoresToFinish(30*1000);
Collection<SolrCore> coreList = new ArrayList<>();
-
- TransientSolrCoreCache transientSolrCoreCache = getTransientCacheHandler();
- // Release observer
- if (transientSolrCoreCache != null) {
- transientSolrCoreCache.close();
- }
+ // Release transient core cache.
+ getTransientCacheHandler().close();
Review comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `SolrCores.close()` indirectly reads without synchronization from `this.transientSolrCoreCacheFactory`. Potentially races with write in method `SolrCores.load(...)`.
Reporting because this access may occur on a background thread.
----------------------------------------------------------------
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
[GitHub] [lucene-solr] ErickErickson commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
ErickErickson commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-723990436
+1
> On Nov 8, 2020, at 3:30 PM, David Smiley <no...@github.com> wrote:
>
>
> So @dsmiley @ErickErickson do we all agree that we throw an exception in in TransientSolrCoreCacheFactory.newInstance() line 60 and we never return null?
>
> +1 definitely
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
>
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#issuecomment-724113057
@dsmiley I pushed a new commit to have a NO OP TransientSolrCacheFactory before SolrCores.load() is called.
This is to have your opinion, whether to keep it or remove it.
If SolrCores is used before calling SolrCores.load() it results in a SolrException in getTransientCacheHandler(). This may happen for current users that previously didn't have to call load() at the very beginning after SolrCores creation. Do you think it could be a behavior backward incompatibility?
----------------------------------------------------------------
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
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r519225448
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -198,35 +188,91 @@ protected SolrCore putCore(CoreDescriptor cd, SolrCore core) {
*
* Put another way, this will not return any names of cores that are lazily loaded but have not been called for yet
* or are transient and either not loaded or have been swapped out.
- *
- * @return List of currently loaded cores.
+ *
+ * @return A unsorted collection.
*/
- Set<String> getLoadedCoreNames() {
- Set<String> set;
-
+ Collection<String> getLoadedCoreNames() {
synchronized (modifyLock) {
- set = new TreeSet<>(cores.keySet());
- if (getTransientCacheHandler() != null) {
- set.addAll(getTransientCacheHandler().getLoadedCoreNames());
- }
+ TransientSolrCoreCache transientCoreCache = getTransientCacheHandler();
Review comment:
There is a comment in TransientSolrCoreCacheFactory.newInstance() line 60 that says a bad config should not prevent the CoreContainer from loading. It logs an continues.
Then SolrCores.getTransientCacheHandler() checks transientCoreCache null. If so it logs an error and returns null.
Before this first PR I tried to replace the null by a NOOP TransientSolrCoreCache singleton. But I realize now that I could use an instance with the parent container. Let's do it, it'll save a lot of null checks.
----------------------------------------------------------------
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