You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2023/01/10 10:20:56 UTC
[GitHub] [solr] bruno-roustant commented on a diff in pull request #1243: SOLR-16591 Make SolrCores pluggable.
bruno-roustant commented on code in PR #1243:
URL: https://github.com/apache/solr/pull/1243#discussion_r1065470256
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -36,100 +36,92 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-class SolrCores {
+/** AKA CoreManager: 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<>();
-
- private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory;
-
- SolrCores(CoreContainer container) {
- this.container = container;
+ private final List<SolrCore> pendingCloses = new ArrayList<>();
+
+ public static SolrCores newSolrCores(CoreContainer coreContainer) {
+ final int transientCacheSize = coreContainer.getConfig().getTransientCacheSize();
+ if (transientCacheSize > 0) {
+ return new TransientSolrCores(coreContainer, transientCacheSize);
+ } else {
+ return new SolrCores(coreContainer);
+ }
}
- protected void addCoreDescriptor(CoreDescriptor p) {
- synchronized (modifyLock) {
- if (p.isTransient()) {
- getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
- } else {
- residentDescriptors.put(p.getName(), p);
- }
- }
+ public SolrCores(CoreContainer container) {
Review Comment:
Shouldn't we keep it package visible? The public newSolrCores() should be preferred, right?
##########
solr/core/src/java/org/apache/solr/core/TransientSolrCores.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.core;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/** A {@link SolrCores} that supports {@link CoreDescriptor#isTransient()}. */
+public class TransientSolrCores extends SolrCores {
+
+ protected final TransientSolrCoreCache transientSolrCoreCache;
+
+ public TransientSolrCores(CoreContainer container, int cacheSize) {
+ super(container);
+ transientSolrCoreCache = new TransientSolrCoreCacheDefault(this, cacheSize);
+ }
+
+ @Override
+ protected void close() {
+ super.close();
+ transientSolrCoreCache.close();
+ }
+
+ @Override
+ public void addCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
+ }
+ } else {
+ super.addCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ public void removeCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
+ }
+ } else {
+ super.removeCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ // Returns the old core if there was a core of the same name.
+ // WARNING! This should be the _only_ place you put anything into the list of transient cores!
+ public SolrCore putCore(CoreDescriptor cd, SolrCore core) {
+ if (cd.isTransient()) {
+ synchronized (modifyLock) {
+ addCoreDescriptor(cd); // cd must always be registered if we register a core
+ return getTransientCacheHandler().addCore(cd.getName(), core);
+ }
+ } else {
+ return super.putCore(cd, core);
+ }
+ }
+
+ @Override
+ public List<String> getLoadedCoreNames() {
+ synchronized (modifyLock) {
+ return distinctSetsUnion(
+ super.getLoadedCoreNames(), getTransientCacheHandler().getLoadedCoreNames());
+ }
+ }
+
+ @Override
+ public List<String> getAllCoreNames() {
+ synchronized (modifyLock) {
+ return distinctSetsUnion(
+ super.getAllCoreNames(), getTransientCacheHandler().getAllCoreNames());
+ }
+ }
+
+ private List<String> distinctSetsUnion(List<String> listA, Set<String> setB) {
+ if (listA.isEmpty()) {
+ return new ArrayList<>(setB);
+ }
+ return distinctSetsUnion(new HashSet<>(listA), setB);
+ }
+
+ /**
+ * Makes the union of two distinct sets.
+ *
+ * @return An unsorted list. This list is a new copy, it can be modified by the caller (e.g. it
+ * can be sorted).
+ */
+ private static <T> List<T> distinctSetsUnion(Set<T> set1, Set<T> set2) {
+ assert areSetsDistinct(set1, set2);
+ List<T> union = new ArrayList<>(set1.size() + set2.size());
+ union.addAll(set1);
+ union.addAll(set2);
+ return union;
+ }
+
+ /** Indicates whether two sets are distinct (intersection is empty). */
+ private static <T> boolean areSetsDistinct(Set<T> set1, Set<T> set2) {
+ return set1.stream().noneMatch(set2::contains);
+ }
+
+ @Override
+ public int getNumLoadedTransientCores() {
+ synchronized (modifyLock) {
+ return getTransientCacheHandler().getLoadedCoreNames().size();
+ }
+ }
+
+ @Override
+ public int getNumUnloadedCores() {
+ synchronized (modifyLock) {
+ return super.getNumUnloadedCores()
+ + getTransientCacheHandler().getAllCoreNames().size()
+ - getTransientCacheHandler().getLoadedCoreNames().size();
+ }
+ }
+
+ @Override
+ public int getNumAllCores() {
+ synchronized (modifyLock) {
+ return super.getNumAllCores() + getTransientCacheHandler().getAllCoreNames().size();
+ }
+ }
+
+ @Override
+ public SolrCore remove(String name) {
+ synchronized (modifyLock) {
+ SolrCore ret = super.remove(name);
+ // It could have been a newly-created core. It could have been a transient core. The
+ // newly-created cores in particular should be checked. It could have been a dynamic core.
+ if (ret == null) {
+ ret = getTransientCacheHandler().removeCore(name);
+ }
+ return ret;
+ }
+ }
+
+ @Override
+ protected SolrCore getLoadedCoreWithoutIncrement(String name) {
+ synchronized (modifyLock) {
+ final var core = super.getLoadedCoreWithoutIncrement(name);
+ return core != null ? core : getTransientCacheHandler().getCore(name);
+ }
+ }
+
+ @Override
+ public boolean isLoaded(String name) {
+ synchronized (modifyLock) {
+ return super.isLoaded(name) || getTransientCacheHandler().containsCore(name);
+ }
+ }
+
+ @Override
+ public CoreDescriptor getCoreDescriptor(String coreName) {
+ synchronized (modifyLock) {
+ CoreDescriptor coreDescriptor = super.getCoreDescriptor(coreName);
+ if (coreDescriptor != null) {
+ return coreDescriptor;
+ }
+ return getTransientCacheHandler().getTransientDescriptor(coreName);
+ }
+ }
+
+ @Override
+ public List<CoreDescriptor> getCoreDescriptors() {
+ synchronized (modifyLock) {
+ List<CoreDescriptor> coreDescriptors = new ArrayList<>(getNumAllCores());
+ coreDescriptors.addAll(super.getCoreDescriptors());
+ coreDescriptors.addAll(getTransientCacheHandler().getTransientDescriptors());
+ return coreDescriptors;
+ }
+ }
+
+ /**
+ * @return the cache holding the transient cores; never null.
+ */
+ private TransientSolrCoreCache getTransientCacheHandler() {
Review Comment:
Maybe inline?
##########
solr/core/src/java/org/apache/solr/core/TransientSolrCores.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.core;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/** A {@link SolrCores} that supports {@link CoreDescriptor#isTransient()}. */
+public class TransientSolrCores extends SolrCores {
+
+ protected final TransientSolrCoreCache transientSolrCoreCache;
+
+ public TransientSolrCores(CoreContainer container, int cacheSize) {
+ super(container);
+ transientSolrCoreCache = new TransientSolrCoreCacheDefault(this, cacheSize);
+ }
+
+ @Override
+ protected void close() {
+ super.close();
+ transientSolrCoreCache.close();
+ }
+
+ @Override
+ public void addCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
+ }
+ } else {
+ super.addCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ public void removeCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
+ }
+ } else {
+ super.removeCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ // Returns the old core if there was a core of the same name.
+ // WARNING! This should be the _only_ place you put anything into the list of transient cores!
+ public SolrCore putCore(CoreDescriptor cd, SolrCore core) {
+ if (cd.isTransient()) {
+ synchronized (modifyLock) {
+ addCoreDescriptor(cd); // cd must always be registered if we register a core
+ return getTransientCacheHandler().addCore(cd.getName(), core);
+ }
+ } else {
+ return super.putCore(cd, core);
+ }
+ }
+
+ @Override
+ public List<String> getLoadedCoreNames() {
+ synchronized (modifyLock) {
+ return distinctSetsUnion(
Review Comment:
We can get rid of distinctSetsUnion and manipulate directly the List returned by the super method because it is a mutable copy. Something like:
```
if (getNumLoadedPermanentCores() == 0) {
return new ArrayList<>(getTransientCacheHandler().getLoadedCoreNames());
}
List<String> coreNames = super.getLoadedCoreNames();
coreNames.addAll(getTransientCacheHandler().getLoadedCoreNames());
return coreNames;
```
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -344,57 +291,47 @@ SolrCore getCoreFromAnyList(String name, boolean incRefCount, UUID coreId) {
}
}
+ /** (internal) Return a core that is already loaded, if it is. NOT incremented! */
+ protected SolrCore getLoadedCoreWithoutIncrement(String name) {
+ synchronized (modifyLock) {
+ return cores.get(name);
+ }
+ }
+
// See SOLR-5366 for why the UNLOAD command needs to know whether a core is actually loaded or
// not, it might have to close the core. However, there's a race condition. If the core happens to
// be in the pending "to close" queue, we should NOT close it in unload core.
- protected boolean isLoadedNotPendingClose(String name) {
- // Just all be synchronized
+ public boolean isLoadedNotPendingClose(String name) {
synchronized (modifyLock) {
- if (cores.containsKey(name)) {
- return true;
+ if (!isLoaded(name)) {
Review Comment:
Good catch
##########
solr/core/src/java/org/apache/solr/core/TransientSolrCores.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.core;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/** A {@link SolrCores} that supports {@link CoreDescriptor#isTransient()}. */
+public class TransientSolrCores extends SolrCores {
+
+ protected final TransientSolrCoreCache transientSolrCoreCache;
+
+ public TransientSolrCores(CoreContainer container, int cacheSize) {
+ super(container);
+ transientSolrCoreCache = new TransientSolrCoreCacheDefault(this, cacheSize);
+ }
+
+ @Override
+ protected void close() {
+ super.close();
+ transientSolrCoreCache.close();
+ }
+
+ @Override
+ public void addCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
+ }
+ } else {
+ super.addCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ public void removeCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
+ }
+ } else {
+ super.removeCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ // Returns the old core if there was a core of the same name.
+ // WARNING! This should be the _only_ place you put anything into the list of transient cores!
+ public SolrCore putCore(CoreDescriptor cd, SolrCore core) {
+ if (cd.isTransient()) {
+ synchronized (modifyLock) {
+ addCoreDescriptor(cd); // cd must always be registered if we register a core
+ return getTransientCacheHandler().addCore(cd.getName(), core);
+ }
+ } else {
+ return super.putCore(cd, core);
+ }
+ }
+
+ @Override
+ public List<String> getLoadedCoreNames() {
+ synchronized (modifyLock) {
+ return distinctSetsUnion(
+ super.getLoadedCoreNames(), getTransientCacheHandler().getLoadedCoreNames());
+ }
+ }
+
+ @Override
+ public List<String> getAllCoreNames() {
+ synchronized (modifyLock) {
+ return distinctSetsUnion(
+ super.getAllCoreNames(), getTransientCacheHandler().getAllCoreNames());
+ }
+ }
+
+ private List<String> distinctSetsUnion(List<String> listA, Set<String> setB) {
+ if (listA.isEmpty()) {
+ return new ArrayList<>(setB);
+ }
+ return distinctSetsUnion(new HashSet<>(listA), setB);
+ }
+
+ /**
+ * Makes the union of two distinct sets.
+ *
+ * @return An unsorted list. This list is a new copy, it can be modified by the caller (e.g. it
+ * can be sorted).
+ */
+ private static <T> List<T> distinctSetsUnion(Set<T> set1, Set<T> set2) {
+ assert areSetsDistinct(set1, set2);
+ List<T> union = new ArrayList<>(set1.size() + set2.size());
+ union.addAll(set1);
+ union.addAll(set2);
+ return union;
+ }
+
+ /** Indicates whether two sets are distinct (intersection is empty). */
+ private static <T> boolean areSetsDistinct(Set<T> set1, Set<T> set2) {
+ return set1.stream().noneMatch(set2::contains);
+ }
+
+ @Override
+ public int getNumLoadedTransientCores() {
+ synchronized (modifyLock) {
+ return getTransientCacheHandler().getLoadedCoreNames().size();
+ }
+ }
+
+ @Override
+ public int getNumUnloadedCores() {
+ synchronized (modifyLock) {
+ return super.getNumUnloadedCores()
+ + getTransientCacheHandler().getAllCoreNames().size()
+ - getTransientCacheHandler().getLoadedCoreNames().size();
+ }
+ }
+
+ @Override
+ public int getNumAllCores() {
+ synchronized (modifyLock) {
+ return super.getNumAllCores() + getTransientCacheHandler().getAllCoreNames().size();
+ }
+ }
+
+ @Override
+ public SolrCore remove(String name) {
+ synchronized (modifyLock) {
+ SolrCore ret = super.remove(name);
+ // It could have been a newly-created core. It could have been a transient core. The
+ // newly-created cores in particular should be checked. It could have been a dynamic core.
+ if (ret == null) {
+ ret = getTransientCacheHandler().removeCore(name);
+ }
+ return ret;
+ }
+ }
+
+ @Override
+ protected SolrCore getLoadedCoreWithoutIncrement(String name) {
+ synchronized (modifyLock) {
+ final var core = super.getLoadedCoreWithoutIncrement(name);
+ return core != null ? core : getTransientCacheHandler().getCore(name);
+ }
+ }
+
+ @Override
+ public boolean isLoaded(String name) {
+ synchronized (modifyLock) {
+ return super.isLoaded(name) || getTransientCacheHandler().containsCore(name);
+ }
+ }
+
+ @Override
+ public CoreDescriptor getCoreDescriptor(String coreName) {
+ synchronized (modifyLock) {
+ CoreDescriptor coreDescriptor = super.getCoreDescriptor(coreName);
+ if (coreDescriptor != null) {
+ return coreDescriptor;
+ }
+ return getTransientCacheHandler().getTransientDescriptor(coreName);
+ }
+ }
+
+ @Override
+ public List<CoreDescriptor> getCoreDescriptors() {
+ synchronized (modifyLock) {
+ List<CoreDescriptor> coreDescriptors = new ArrayList<>(getNumAllCores());
Review Comment:
Same as line 80.
##########
solr/core/src/java/org/apache/solr/core/TransientSolrCores.java:
##########
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.core;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+/** A {@link SolrCores} that supports {@link CoreDescriptor#isTransient()}. */
+public class TransientSolrCores extends SolrCores {
+
+ protected final TransientSolrCoreCache transientSolrCoreCache;
+
+ public TransientSolrCores(CoreContainer container, int cacheSize) {
+ super(container);
+ transientSolrCoreCache = new TransientSolrCoreCacheDefault(this, cacheSize);
+ }
+
+ @Override
+ protected void close() {
+ super.close();
+ transientSolrCoreCache.close();
+ }
+
+ @Override
+ public void addCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
+ }
+ } else {
+ super.addCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ public void removeCoreDescriptor(CoreDescriptor p) {
+ if (p.isTransient()) {
+ synchronized (modifyLock) {
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
+ }
+ } else {
+ super.removeCoreDescriptor(p);
+ }
+ }
+
+ @Override
+ // Returns the old core if there was a core of the same name.
+ // WARNING! This should be the _only_ place you put anything into the list of transient cores!
+ public SolrCore putCore(CoreDescriptor cd, SolrCore core) {
+ if (cd.isTransient()) {
+ synchronized (modifyLock) {
+ addCoreDescriptor(cd); // cd must always be registered if we register a core
+ return getTransientCacheHandler().addCore(cd.getName(), core);
+ }
+ } else {
+ return super.putCore(cd, core);
+ }
+ }
+
+ @Override
+ public List<String> getLoadedCoreNames() {
+ synchronized (modifyLock) {
+ return distinctSetsUnion(
+ super.getLoadedCoreNames(), getTransientCacheHandler().getLoadedCoreNames());
+ }
+ }
+
+ @Override
+ public List<String> getAllCoreNames() {
+ synchronized (modifyLock) {
+ return distinctSetsUnion(
Review Comment:
Same as line 80.
##########
solr/core/src/java/org/apache/solr/core/TransientSolrCoreCacheDefault.java:
##########
@@ -119,38 +115,25 @@ private void onEvict(SolrCore core) {
}
}
- private int getConfiguredCacheMaxSize(CoreContainer container) {
- int configuredCacheMaxSize = NodeConfig.NodeConfigBuilder.DEFAULT_TRANSIENT_CACHE_SIZE;
- NodeConfig cfg = container.getNodeConfig();
- if (cfg.getTransientCachePluginInfo() == null) {
- // Still handle just having transientCacheSize defined in the body of solr.xml
- // not in a transient handler clause.
- configuredCacheMaxSize = cfg.getTransientCacheSize();
- } else {
- NamedList<?> args = cfg.getTransientCachePluginInfo().initArgs;
- Object obj = args.get("transientCacheSize");
- if (obj != null) {
- configuredCacheMaxSize = (int) obj;
- }
+ private int getConfiguredCacheMaxSize(NamedList<?> initArgs) {
Review Comment:
It seems this method is not used anymore.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org