ChrisHegarty commented on code in PR #14275:
URL: https://github.com/apache/lucene/pull/14275#discussion_r1971253531
##########
lucene/core/src/java/org/apache/lucene/util/NamedSPILoader.java:
##########
@@ -64,18 +64,24 @@ public NamedSPILoader(Class<S> clazz, ClassLoader
classloader) {
*/
public void reload(ClassLoader classloader) {
Objects.requireNonNull(classloader, "classloader");
- final LinkedHashMap<String, S> services = new
LinkedHashMap<>(this.services);
+ final Map<String, S> services = this.services;
+ final LinkedHashMap<String, S> newServices = new LinkedHashMap<>();
for (final S service : ServiceLoader.load(clazz, classloader)) {
final String name = service.getName();
// only add the first one for each name, later services will be ignored
- // this allows to place services before others in classpath to make
- // them used instead of others
- if (!services.containsKey(name)) {
- checkServiceName(name);
- services.put(name, service);
+ // unless the later-found service allows to replace the previous.
+ var prevNew = newServices.get(name);
Review Comment:
> what is the current behaviour is the same service is registered multiple
times?
First one wins. Subsequent ones are ignored. As it was prior to this
proposed change.
> ... it's not deterministic which one?
For applications deployed on the classpath it is deterministic. Since the
classpath is an ordered sequence of directories and jars, the order of service
finding is: 1) the order of the classpath - earlier jars/dirs are encountered
before later ones, and 2) the order of services defined in the
`META-INF/services`. So applications and services on the classpath are
deterministic.
For applications deployed as modules (as Elasticsearch is), the module layer
is searched for modules that `provides` the required service. "The ordering of
modules in same class loader, or the ordering of modules in a module layer, is
not defined." [1]. This is where the non-determinism is coming from.
> I was wondering if the replace default behaviour introduces a potential
breaking behaviour for existing users.
By default, behaviour remains unchanged. The default value retuned by
`replace` is `false` - "do not replace". Services that are provided by lucene
modules/jar will not replace or override that of the user. And since this is
new, so the `replace` method returns `false`, user services will not replace
lucene services.
> I guess though that is granted if we want to make things deterministic:
there are things that may have worked almost by accident before this change,
that now require overriding replace explicitly?
For modules you are exactly right. If it works today with modules, then it
is a little fragile and non-deterministic.
Going forward, services provide by Lucene jars/modules should return
`replace` `false`, that way users can programmatically select their service
over the one provided by Lucene, irrespective of module or classpath ordering.
However, classpath ordering will be affected by this, if a user provides a
`replace` `true`. But this is exactly what we want.
[1]
https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/ServiceLoader.html#load(java.lang.Class,java.lang.ClassLoader)
--
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...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
