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 20:09:39 UTC
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #2066: SOLR-14975: Optimize CoreContainer.getAllCoreNames and getLoadedCoreNames.
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