[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1033: SOLR-13965: Use Plugin to add new expressions to GraphHandler
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_r373601779
##
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 pluginInfos =
core.getSolrConfig().getPluginInfos(Expressible.class.getName());
+
+// Check deprecated approach.
Object functionMappingsObj = initArgs.get("streamFunctions");
if(null != functionMappingsObj){
+ log.warn("solrconfig.xml: is deprecated for adding
additional streaming functions to GraphHandler.");
NamedList functionMappings = (NamedList)functionMappingsObj;
for(Entry functionMapping : functionMappings) {
String key = functionMapping.getKey();
PluginInfo pluginInfo = new PluginInfo(key,
Collections.singletonMap("class", functionMapping.getValue()));
-
-if (pluginInfo.pkgName == null) {
- Class 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);
}
+}
+for (PluginInfo pluginInfo : pluginInfos) {
+ if (pluginInfo.pkgName != null) {
+ExpressibleHolder holder = new ExpressibleHolder(pluginInfo, core,
SolrConfig.classVsSolrPluginInfo.get(Expressible.class));
+streamFactory.withFunctionName(pluginInfo.name,
+() -> holder.getClazz());
+ } else {
+Class clazz =
core.getMemClassLoader().findClass(pluginInfo.className, Expressible.class);
Review comment:
https://github.com/epugh/lucene-solr/pull/1 offers to merge in the current
`origin/master` into this PR and then adds usage of the factored out
`StreamHandler.addExpressiblePlugins` method from SOLR-13965.01.patch file.
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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1033: SOLR-13965: Use Plugin to add new expressions to GraphHandler
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_r370262941
##
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 pluginInfos =
core.getSolrConfig().getPluginInfos(Expressible.class.getName());
+
+// Check deprecated approach.
Object functionMappingsObj = initArgs.get("streamFunctions");
if(null != functionMappingsObj){
+ log.warn("solrconfig.xml: is deprecated for adding
additional streaming functions to GraphHandler.");
NamedList functionMappings = (NamedList)functionMappingsObj;
for(Entry functionMapping : functionMappings) {
String key = functionMapping.getKey();
PluginInfo pluginInfo = new PluginInfo(key,
Collections.singletonMap("class", functionMapping.getValue()));
-
-if (pluginInfo.pkgName == null) {
- Class 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);
}
+}
+for (PluginInfo pluginInfo : pluginInfos) {
+ if (pluginInfo.pkgName != null) {
+ExpressibleHolder holder = new ExpressibleHolder(pluginInfo, core,
SolrConfig.classVsSolrPluginInfo.get(Expressible.class));
Review comment:
> ... feel free to file a new JIRA ...
Opened https://issues.apache.org/jira/browse/SOLR-14212 for this.
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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1033: SOLR-13965: Use Plugin to add new expressions to GraphHandler
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_r368076257
##
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 pluginInfos =
core.getSolrConfig().getPluginInfos(Expressible.class.getName());
+
+// Check deprecated approach.
Object functionMappingsObj = initArgs.get("streamFunctions");
if(null != functionMappingsObj){
+ log.warn("solrconfig.xml: is deprecated for adding
additional streaming functions to GraphHandler.");
NamedList functionMappings = (NamedList)functionMappingsObj;
for(Entry functionMapping : functionMappings) {
String key = functionMapping.getKey();
PluginInfo pluginInfo = new PluginInfo(key,
Collections.singletonMap("class", functionMapping.getValue()));
-
-if (pluginInfo.pkgName == null) {
- Class 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);
}
+}
+for (PluginInfo pluginInfo : pluginInfos) {
+ if (pluginInfo.pkgName != null) {
+ExpressibleHolder holder = new ExpressibleHolder(pluginInfo, core,
SolrConfig.classVsSolrPluginInfo.get(Expressible.class));
+streamFactory.withFunctionName(pluginInfo.name,
+() -> holder.getClazz());
+ } else {
+Class clazz =
core.getMemClassLoader().findClass(pluginInfo.className, Expressible.class);
Review comment:
> Since this code is duplicated between Stream & Graph, can we factor it out
into a common method?
Attached SOLR-13965.01.patch to the JIRA ticket.
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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1033: SOLR-13965: Use Plugin to add new expressions to GraphHandler
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 pluginInfos =
core.getSolrConfig().getPluginInfos(Expressible.class.getName());
+
+// Check deprecated approach.
Object functionMappingsObj = initArgs.get("streamFunctions");
if(null != functionMappingsObj){
+ log.warn("solrconfig.xml: is deprecated for adding
additional streaming functions to GraphHandler.");
NamedList functionMappings = (NamedList)functionMappingsObj;
for(Entry functionMapping : functionMappings) {
String key = functionMapping.getKey();
PluginInfo pluginInfo = new PluginInfo(key,
Collections.singletonMap("class", functionMapping.getValue()));
-
-if (pluginInfo.pkgName == null) {
- Class 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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1033: SOLR-13965: Use Plugin to add new expressions to GraphHandler
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_r362065317
##
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 pluginInfos =
core.getSolrConfig().getPluginInfos(Expressible.class.getName());
+
+// Check deprecated approach.
Object functionMappingsObj = initArgs.get("streamFunctions");
if(null != functionMappingsObj){
+ log.warn("solrconfig.xml: is deprecated for adding
additional streaming functions to GraphHandler.");
NamedList functionMappings = (NamedList)functionMappingsObj;
for(Entry functionMapping : functionMappings) {
String key = functionMapping.getKey();
PluginInfo pluginInfo = new PluginInfo(key,
Collections.singletonMap("class", functionMapping.getValue()));
-
-if (pluginInfo.pkgName == null) {
- Class 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);
}
+}
+for (PluginInfo pluginInfo : pluginInfos) {
+ if (pluginInfo.pkgName != null) {
+ExpressibleHolder holder = new ExpressibleHolder(pluginInfo, core,
SolrConfig.classVsSolrPluginInfo.get(Expressible.class));
+streamFactory.withFunctionName(pluginInfo.name,
+() -> holder.getClazz());
+ } else {
+Class clazz =
core.getMemClassLoader().findClass(pluginInfo.className, Expressible.class);
Review comment:
Thanks @epugh for resolving the merge conflicts! The previous
`getResourceLoader` is now replaced with `getMemClassLoader` here, just
checking if that's intended or accidental perhaps, not yet looked what the
difference (if any) between the two methods' behaviour 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:
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
[GitHub] [lucene-solr] cpoerschke commented on a change in pull request #1033: SOLR-13965: Use Plugin to add new expressions to GraphHandler
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_r355494075
##
File path: solr/core/src/java/org/apache/solr/handler/GraphHandler.java
##
@@ -87,14 +88,10 @@ public void inform(SolrCore core) {
}
// This pulls all the overrides and additions from the config
-Object functionMappingsObj = initArgs.get("streamFunctions");
Review comment:
I wonder with backwards compatibility in mind, if for a period of time
`streamFunctions` might be deprecated but still supported alongside the new
`expressible` equivalent. Users would see the 'deprecated' warnings being
logged and could transition callers ahead of `streamFunctions` support being
taken away in a later version. What do you think?
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
