cpoerschke commented on a change in pull request #1033: SOLR-13965: Use Plugin
to add new expressions to GraphHandler
URL: https://github.com/apache/lucene-solr/pull/1033#discussion_r362070735
##########
File path: solr/core/src/java/org/apache/solr/handler/GraphHandler.java
##########
@@ -92,24 +104,29 @@ public void inform(SolrCore core) {
}
// This pulls all the overrides and additions from the config
+ List<PluginInfo> pluginInfos =
core.getSolrConfig().getPluginInfos(Expressible.class.getName());
+
+ // Check deprecated approach.
Object functionMappingsObj = initArgs.get("streamFunctions");
if(null != functionMappingsObj){
+ log.warn("solrconfig.xml: <streamFunctions> is deprecated for adding
additional streaming functions to GraphHandler.");
NamedList<?> functionMappings = (NamedList<?>)functionMappingsObj;
for(Entry<String,?> functionMapping : functionMappings) {
String key = functionMapping.getKey();
PluginInfo pluginInfo = new PluginInfo(key,
Collections.singletonMap("class", functionMapping.getValue()));
-
- if (pluginInfo.pkgName == null) {
- Class<? extends Expressible> clazz =
core.getResourceLoader().findClass((String) functionMapping.getValue(),
- Expressible.class);
- streamFactory.withFunctionName(key, clazz);
- } else {
- StreamHandler.ExpressibleHolder holder = new
StreamHandler.ExpressibleHolder(pluginInfo, core,
SolrConfig.classVsSolrPluginInfo.get(Expressible.class));
- streamFactory.withFunctionName(key, () -> holder.getClazz());
- }
-
+ pluginInfos.add(pluginInfo);
Review comment:
The `pluginInfos.add` here raises an interesting question w.r.t. both
`streamFunctions` and `expressible` being present and what should happen if
both were to configure the same 'key' ...
My first thought was that simply disallowing the combination is easiest but
actually that's not a good idea since `streamFunctions` is on a handler level
whereas `expressible` is on the config level and so the presence of existing
`expressible` elements would result in the also already present
`streamFunctions` no longer working, which would not be very user backwards
compatibility friendly.
If use of both elements is supported though then the deprecated variant
being appended to the list could (at least theoretically) result in the
deprecated `streamFunctions` configuration for name 'foo' using 'FooClass'
overriding (for the GraphHandler only) any `expressible` mapping from name
'foo' to 'BarClass'. But then again, adding validation e.g. to reject any
`streamFunctions` elements that are already present-and-different in
`expressible` would add code complexity.
@joel-bernstein would you have any thoughts on this? Beyond the scope of
this change here the question essentially is about multiple
[StreamFactory.withFunctionName](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.4.0/solr/solrj/src/java/org/apache/solr/client/solrj/io/stream/expr/StreamFactory.java#L85-L88)
calls for the same function name.
----------------------------------------------------------------
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:
us...@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
