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/07/12 06:21:44 UTC
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1666: SOLR-14155: Load all other SolrCore plugins from packages
dsmiley commented on a change in pull request #1666:
URL: https://github.com/apache/lucene-solr/pull/1666#discussion_r453258414
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1588,20 +1576,28 @@ private CoreDescriptor reloadCoreDescriptor(CoreDescriptor oldDesc) {
return ret;
}
+ public void reload(String name) {
Review comment:
Very much needs a javadoc line saying *what* it is that is being reloaded. My initial and incorrect thought was the CoreContainer itself.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrConfig.java
##########
@@ -971,6 +974,21 @@ public RequestParams getRequestParams() {
return requestParams;
}
+ /**
+ * The version of package that should be loaded for a given package name
+ * This information is stored in the params.json in the same configset
+ * If params.json is absent or there is no corresponding version specified for a given package,
+ * the latest is used
Review comment:
The code shows null can be returned but javadoc says "latest".
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1588,20 +1576,28 @@ private CoreDescriptor reloadCoreDescriptor(CoreDescriptor oldDesc) {
return ret;
}
+ public void reload(String name) {
+ reload(name, null);
+ }
/**
* Recreates a SolrCore.
* While the new core is loading, requests will continue to be dispatched to
* and processed by the old core
*
* @param name the name of the SolrCore to reload
+ * @param coreId The unique identifier of the core
Review comment:
This is new. Can you explain why this new ID is needed?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -626,25 +615,26 @@ public void deleteNonSnapshotIndexFiles(String indexDirPath) throws IOException
}
+ @SuppressWarnings("unchecked")
private void initListeners() {
final Class<SolrEventListener> clazz = SolrEventListener.class;
final String label = "Event Listener";
for (PluginInfo info : solrConfig.getPluginInfos(SolrEventListener.class.getName())) {
final String event = info.attributes.get("event");
if ("firstSearcher".equals(event)) {
- SolrEventListener obj = createInitInstance(info, clazz, label, null);
+ PluginHolder<SolrEventListener> obj = (PluginHolder<SolrEventListener>)PackagePluginHolder.createHolder(info, this, SolrEventListener.class, label);
firstSearcherListeners.add(obj);
- log.debug("[{}] Added SolrEventListener for firstSearcher: [{}]", logid, obj);
+ log.debug("[{}] Added SolrEventListener for firstSearcher: [{}]", logid, obj.get());
} else if ("newSearcher".equals(event)) {
- SolrEventListener obj = createInitInstance(info, clazz, label, null);
+ PluginHolder<SolrEventListener> obj = (PluginHolder<SolrEventListener>)PackagePluginHolder.createHolder(info, this, SolrEventListener.class, label);
newSearcherListeners.add(obj);
- log.debug("[{}] Added SolrEventListener for newSearcher: [{}]", logid, obj);
+ log.debug("[{}] Added SolrEventListener for newSearcher: [{}]", logid, obj.get());
}
}
}
- final List<SolrEventListener> firstSearcherListeners = new ArrayList<>();
- final List<SolrEventListener> newSearcherListeners = new ArrayList<>();
+ final List<PluginHolder<SolrEventListener>> firstSearcherListeners = new ArrayList<>();
Review comment:
Can we use a PluginBag instead of List<PluginHolder>, which is shorter and more consistent with how SolrCore tracks some plugins like SearchComponents, URPs, and RequestHandlers?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -191,6 +175,11 @@
private String name;
private String logid; // used to show what name is set
+ /**
+ * A unique id to differentiate multiple instances of the same core
+ * If we reload a core, the name remains same , but the id will be new
+ */
+ public final UUID uniqueId = UUID.randomUUID();
Review comment:
If this ID is completely internal to the node (never passed in as a parameter), then can't we just use some AtomicLong?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -626,25 +615,26 @@ public void deleteNonSnapshotIndexFiles(String indexDirPath) throws IOException
}
+ @SuppressWarnings("unchecked")
private void initListeners() {
final Class<SolrEventListener> clazz = SolrEventListener.class;
final String label = "Event Listener";
for (PluginInfo info : solrConfig.getPluginInfos(SolrEventListener.class.getName())) {
final String event = info.attributes.get("event");
if ("firstSearcher".equals(event)) {
- SolrEventListener obj = createInitInstance(info, clazz, label, null);
+ PluginHolder<SolrEventListener> obj = (PluginHolder<SolrEventListener>)PackagePluginHolder.createHolder(info, this, SolrEventListener.class, label);
firstSearcherListeners.add(obj);
- log.debug("[{}] Added SolrEventListener for firstSearcher: [{}]", logid, obj);
+ log.debug("[{}] Added SolrEventListener for firstSearcher: [{}]", logid, obj.get());
Review comment:
Maybe it's okay but the `obj.get()` worries me a little. As a reader of the code here at the caller, it seems suspicious to evaluate a Supplier on a debug log statement. Maybe it's not necessarily "safe" (lazy) and it should only be fetched if really needed. For a log statement, lets just pass "obj" (PluginHolder) and add a decent toString to the PluginHolder?
##########
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##########
@@ -66,27 +66,39 @@ public PluginInfo(String type, Map<String, String> attrs, @SuppressWarnings({"ra
* This checks if it is a package name prefixed classname.
* the return value has first = package name and second = class name
*/
- public static Pair<String,String > parseClassName(String name) {
- String pkgName = null;
- String className = name;
- if (name != null) {
+ public static CName parseClassName(String name) {
+ return new CName(name);
+ }
+
+ public static class CName {
+ public final String pkg;
+ public final String className;
+
+ public CName(String name) {
+ if (name == null) {
Review comment:
Is a null name even valid? Maybe throw IllegalArgumentException if not instead?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -626,25 +615,26 @@ public void deleteNonSnapshotIndexFiles(String indexDirPath) throws IOException
}
+ @SuppressWarnings("unchecked")
private void initListeners() {
final Class<SolrEventListener> clazz = SolrEventListener.class;
final String label = "Event Listener";
for (PluginInfo info : solrConfig.getPluginInfos(SolrEventListener.class.getName())) {
final String event = info.attributes.get("event");
if ("firstSearcher".equals(event)) {
- SolrEventListener obj = createInitInstance(info, clazz, label, null);
+ PluginHolder<SolrEventListener> obj = (PluginHolder<SolrEventListener>)PackagePluginHolder.createHolder(info, this, SolrEventListener.class, label);
Review comment:
Can the createHolder method be declared in such a way (with generics) that the cast here isn't needed?
##########
File path: solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java
##########
@@ -250,7 +250,7 @@ private boolean deployPackage(SolrPackageInstance packageInstance, boolean pegTo
// Set the package version in the collection's parameters
try {
SolrCLI.postJsonToSolr(solrClient, PackageUtils.getCollectionParamsPath(collection),
- "{set:{PKG_VERSIONS:{" + packageInstance.name+": '" + (pegToLatest? PackagePluginHolder.LATEST: packageInstance.version)+"'}}}");
+ "{set:{PKG_VERSIONS:{" + packageInstance.name+": '" + (pegToLatest? SolrConfig.LATEST: packageInstance.version)+"'}}}");
Review comment:
Moving this constant to SolrConfig seems odd. If the constant is specific to packages/versions then I think it belongs in the "pkg" Java package somewhere.
##########
File path: solr/core/src/java/org/apache/solr/update/UpdateHandler.java
##########
@@ -48,25 +46,26 @@
protected final SchemaField idField;
protected final FieldType idFieldType;
- protected Vector<SolrEventListener> commitCallbacks = new Vector<>();
- protected Vector<SolrEventListener> softCommitCallbacks = new Vector<>();
- protected Vector<SolrEventListener> optimizeCallbacks = new Vector<>();
+ protected Vector<PluginHolder<SolrEventListener>> commitCallbacks = new Vector<>();
+ protected Vector<PluginHolder<SolrEventListener>> softCommitCallbacks = new Vector<>();
+ protected Vector<PluginHolder<SolrEventListener>> optimizeCallbacks = new Vector<>();
protected final UpdateLog ulog;
protected SolrMetricsContext solrMetricsContext;
+ @SuppressWarnings("unchecked")
Review comment:
you didn't add casts so this annotation doesn't seem necessary
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##########
@@ -96,15 +97,32 @@ private synchronized void invokeListeners(PackageLoader.Package pkg) {
public interface Listener {
- /**Name of the package or null to loisten to all package changes
+ /**Name of the package or null to listen to all package changes
*/
String packageName();
PluginInfo pluginInfo();
- void changed(PackageLoader.Package pkg);
+ void changed(PackageLoader.Package pkg, Ctx ctx);
PackageLoader.Package.Version getPackageVersion();
+ class Ctx {
+ private Map<String, Runnable > runLater;
+ public void runLater(String name, Runnable runnable ){
+ if(runLater == null) runLater = new LinkedHashMap<>();
+ runLater.put(name, runnable);
+ }
+ private void runLaterTasks(){
+ if(runLater == null) return;
+ new Thread(() -> runLater.forEach((s, runnable) -> {
Review comment:
Woah; probably needs more thought. When is "later" (after what)? An Executor should probably be used. Does this really need to be async? Maybe "sometimes" (some Runnables) so maybe the supplier of the Runnable should handle it by using its own Executor.
##########
File path: solr/core/src/java/org/apache/solr/update/UpdateHandler.java
##########
@@ -77,33 +76,35 @@ private void parseEventListeners() {
* Call the {@link SolrCoreAware#inform(SolrCore)} on all the applicable registered listeners.
*/
public void informEventListeners(SolrCore core) {
- for (SolrEventListener listener: commitCallbacks) {
- if (listener instanceof SolrCoreAware) {
- ((SolrCoreAware) listener).inform(core);
+ for (PluginHolder<SolrEventListener> listener: commitCallbacks) {
Review comment:
you may want to auto-format the code in this method; I see some extra spaces and inconsistent use of spaces
##########
File path: solr/core/src/java/org/apache/solr/search/CacheConfig.java
##########
@@ -64,7 +67,7 @@ public CacheConfig() {}
@SuppressWarnings({"rawtypes"})
public CacheConfig(Class<? extends SolrCache> clazz, Map<String,String> args, CacheRegenerator regenerator) {
- this.clazz = clazz;
+ this.clazz = () -> clazz;
Review comment:
Why lazy-evaluate the class? In the issue description, the solrconfig caches were not listed as hot-reloadable, and so I'm surprised to see changes here.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##########
@@ -96,15 +97,32 @@ private synchronized void invokeListeners(PackageLoader.Package pkg) {
public interface Listener {
- /**Name of the package or null to loisten to all package changes
+ /**Name of the package or null to listen to all package changes
*/
String packageName();
PluginInfo pluginInfo();
- void changed(PackageLoader.Package pkg);
+ void changed(PackageLoader.Package pkg, Ctx ctx);
PackageLoader.Package.Version getPackageVersion();
+ class Ctx {
+ private Map<String, Runnable > runLater;
Review comment:
needs a comment as to what the "String" key is
----------------------------------------------------------------
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