dsmiley commented on code in PR #3172:
URL: https://github.com/apache/solr/pull/3172#discussion_r1948343274
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1129,7 +1129,7 @@ protected SolrCore(
initWriters();
qParserPlugins.init(QParserPlugin.standardPlugins, this);
valueSourceParsers.init(ValueSourceParser.standardValueSourceParsers,
this);
- transformerFactories.init(TransformerFactory.defaultFactories, this);
+
transformerFactories.init(TransformerFactory.builtIns(coreContainer.getConfig()),
this);
Review Comment:
I like the name "builtIns" best so I chose it. "implicits" would be my
second choice. I like a general name, thus not including "factories" as some
places where we would use this pattern are not even "factories". "default"
isn't terrible but less good.
##########
solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java:
##########
Review Comment:
There are some other TransformerFactory impls not listed here; 2 for
QueryElevationComponent which weirdly self-register when QEC loads, and the
other is LTR. I could expand the scope of this PR but wanted to start simpler
to get the conversation going.
##########
solr/core/src/java/org/apache/solr/response/transform/ChildDocTransformerFactory.java:
##########
@@ -18,6 +18,7 @@
import static org.apache.solr.schema.IndexSchema.NEST_PATH_FIELD_NAME;
+import jakarta.inject.Named;
Review Comment:
To solve the problem of identifying the name for plugins, I found this
annotation, which seems quite suitable. I read further about it when looking
at HK2 (a dependency of ours) lightweight dependency-injection framework that
uses the annotations in that JAR/package. This PR doesn't use HK2 BTW.
##########
solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java:
##########
@@ -105,18 +107,17 @@ default boolean mayModifyValue() {
}
}
- public static final Map<String, TransformerFactory> defaultFactories = new
HashMap<>(9, 1.0f);
+ // loaded once, based on the node
+ private static Map<String, TransformerFactory> builtIns;
- static {
- defaultFactories.put("explain", new ExplainAugmenterFactory());
- defaultFactories.put("value", new ValueAugmenterFactory());
- defaultFactories.put("docid", new DocIdAugmenterFactory());
- defaultFactories.put("shard", new ShardAugmenterFactory());
- defaultFactories.put("child", new ChildDocTransformerFactory());
- defaultFactories.put("subquery", new SubQueryAugmenterFactory());
- defaultFactories.put("json", new RawValueTransformerFactory("json"));
- defaultFactories.put("xml", new RawValueTransformerFactory("xml"));
- defaultFactories.put("geo", new GeoTransformerFactory());
- defaultFactories.put("core", new CoreAugmenterFactory());
+ /** (for internal use) */
+ public static synchronized Map<String, TransformerFactory>
builtIns(NodeConfig nodeConfig) {
Review Comment:
Since the lazy loaded list is initialized based on NodeConfig (basically
CoreContainer), admittedly there's a test stability risk since NodeConfig
ClassLoader classpath of the first test that runs will "win". For the built-in
plugins, let's just not do anything that would disturb that list. If we wanted
to test that someone could add a plugin in a JAR, that would be a high level
integration test using Docker or BATS. Not unit tests.
##########
solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java:
##########
@@ -105,18 +107,17 @@ default boolean mayModifyValue() {
}
}
- public static final Map<String, TransformerFactory> defaultFactories = new
HashMap<>(9, 1.0f);
+ // loaded once, based on the node
+ private static Map<String, TransformerFactory> builtIns;
Review Comment:
no static initializer anymore; no class loading deadlock risk. Right
@uschindler ?
##########
solr/core/src/java/org/apache/solr/response/transform/CoreAugmenterFactory.java:
##########
@@ -17,9 +17,11 @@
package org.apache.solr.response.transform;
+import jakarta.inject.Named;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.request.SolrQueryRequest;
+@Named("core")
public class CoreAugmenterFactory extends TransformerFactory {
Review Comment:
`<RANT>` Why the hell didn't we name TransformerFactory as its rightful
name, DocTransformerFactory, and name it's implementations as such as well? I
don't even need to know who/why; it's just wrong. We have sinned and need to
make amends someday.`</RANT>`
##########
solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java:
##########
@@ -105,18 +107,17 @@ default boolean mayModifyValue() {
}
}
- public static final Map<String, TransformerFactory> defaultFactories = new
HashMap<>(9, 1.0f);
+ // loaded once, based on the node
+ private static Map<String, TransformerFactory> builtIns;
- static {
- defaultFactories.put("explain", new ExplainAugmenterFactory());
- defaultFactories.put("value", new ValueAugmenterFactory());
- defaultFactories.put("docid", new DocIdAugmenterFactory());
- defaultFactories.put("shard", new ShardAugmenterFactory());
- defaultFactories.put("child", new ChildDocTransformerFactory());
- defaultFactories.put("subquery", new SubQueryAugmenterFactory());
- defaultFactories.put("json", new RawValueTransformerFactory("json"));
- defaultFactories.put("xml", new RawValueTransformerFactory("xml"));
- defaultFactories.put("geo", new GeoTransformerFactory());
- defaultFactories.put("core", new CoreAugmenterFactory());
+ /** (for internal use) */
+ public static synchronized Map<String, TransformerFactory>
builtIns(NodeConfig nodeConfig) {
Review Comment:
Anyway, the use of NodeConfig here thus adds a capability of config-less
addition of a plugin (i.e. just add a JAR for a custom DocTransformer; no need
to edit solrconfig.xml / configSet).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
