Load pre-loaded lookups via SPI, rather than hard-code in Interpolator. #396

tbwork · 2020-08-10T06:38:07Z

issue link: Provide a way to add customized StrLookUps in Interpolator initialization.

test custom lookup fix support custom lookup

tbwork · 2020-08-10T09:53:52Z

What's wrong with the CI? The change should have nothing to do with the error. :)

jvz · 2020-08-10T13:55:19Z

No worries about the Travis check. The GitHub Actions checks are the important ones. We need to disable our old Travis checks.

rgoers · 2020-08-10T15:43:45Z

The only concern I have with this is the overhead of ServiceLoader during startup. I recently had to make ServiceLoader run asynchronously during startup while loading Custom ThreadContextDataProviders. This will likely cause the same problem. We should make that more generic so that components such as them can register their service for asynchronous initialization. But that would be done in a different PR.

jvz

This might make more sense being named something like ServiceLookup as it specifically queries service providers. Custom sounds too generic.

rgoers · 2020-08-10T16:11:01Z

@jvz I dislike ServiceLookup as it is describing its implementation method, not its purpose, which is to allow a way for users to hook into a Lookup that is configured to run before any standard plugins have been processed.

Perhaps that is an indication that the mechanism provided here is incorrect. Perhaps the service loader should be looking for Lookup Plugins that are also declared as services. We could replace the way all these lookups are loaded if we do that.

rgoers

See my previous comment. The more I think about this the more it seems all the pre-loaded Lookups should be loaded via ServiceLoader. So custom Lookups that want to be included in this list would just register themselves as services.

tbwork · 2020-08-10T17:02:36Z

@rgoers Agree! This PR is intended to do that as you proposed. Hope this could be supported ASAP :) . Have a nice day.

tbwork · 2020-08-10T17:11:58Z

@rgoers Do you mean all the pre-loaded Lookups should be loaded via ServiceLoader? I am glad to contribute to this part if you do not mind. :)

rgoers · 2020-08-10T18:28:07Z

Yes, that is exactly what I mean. Feel free to modify this PR to do that. In essence, StrLookup would become the service interface and every plugin to be pre-loaded would be listed in the services file instead of hard coded in Interpolator.

tbwork · 2020-08-11T02:59:48Z

@rgoers ok.

tbwork · 2020-08-11T07:21:04Z

@jvz @rgoers It seems the Log4jPlugins class is moved from org.apache.logging.log4j.core.plugins. However in Activator class, Log4jPlugins is still imported as "org.apache.logging.log4j.core.plugins.Log4jPlugins".

tbwork

Changes are reviewed by Tommy.Tesla.

tbwork · 2020-08-14T04:04:02Z

@rgoers @jvz Hi, are you busy? Could you get round to take a review and merge :).

jvz · 2020-08-15T19:04:19Z

log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/JndiLookup.java

+        // JNDI check
+        try {
+            // [LOG4J2-703] We might be on Android
+            Loader.newCheckedInstanceOf("org.apache.logging.log4j.core.lookup.JndiLookup", StrLookup.class);


This is trying to load itself here?

Hah, my fault. Your r right. How about just removing them? It is not necessary in terms of SPI mode.

jvz · 2020-08-15T19:09:31Z

log4j-docker/src/main/java/org/apache/logging/log4j/docker/DockerLookup.java

+
+        try {
+            // docker check.
+            Loader.newCheckedInstanceOf("org.apache.logging.log4j.docker.DockerLookup", StrLookup.class);


These checks are attempting to load the class they're in. They won't load properly when SystemLoader.load() tries to load the class itself due to the static references to optional classpath code. This check you've added is only useful for code outside this exact file. The same goes for the other classes you've added these to.

jvz · 2020-08-15T19:09:41Z

log4j-kubernetes/src/main/java/org/apache/logging/log4j/kubernetes/KubernetesLookup.java

    }

    KubernetesLookup(Pod pod, Namespace namespace, URL masterUrl) {
        this.pod = pod;
        this.namespace = namespace;
        this.masterUrl = masterUrl;
+
+        try {
+            Loader.newCheckedInstanceOf("org.apache.logging.log4j.kubernetes.KubernetesLookup", StrLookup.class);


jvz · 2020-08-15T19:09:49Z

...g-client/src/main/java/org/apache/logging/log4j/spring/cloud/config/client/SpringLookup.java

@@ -40,9 +40,21 @@
    private static final Pattern DEFAULT_PATTERN = Pattern.compile(DEFAULT + PATTERN);

    public SpringLookup() {
+        try {
+            //Spring check.
+            Loader.newCheckedInstanceOf("org.apache.logging.log4j.spring.cloud.config.client.SpringLookup", StrLookup.class);


jvz · 2020-08-15T19:10:07Z

log4j-web/src/main/java/org/apache/logging/log4j/web/WebLookup.java

+        super();
+        if (Constants.IS_WEB_APP) {
+            try {
+                Loader.newCheckedInstanceOf("org.apache.logging.log4j.web.WebLookup", StrLookup.class);


jvz · 2020-08-15T19:11:40Z

...-core/src/main/java/org/apache/logging/log4j/core/lookup/JmxRuntimeInputArgumentsLookup.java

+        // JMX check
+        try {
+            // We might be on Android
+            Loader.newCheckedInstanceOf("org.apache.logging.log4j.core.lookup.JmxRuntimeInputArgumentsLookup",


jvz · 2020-08-15T19:12:31Z

log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/Interpolator.java

+            /*
+             *   Comparator for ordering pure StrLookup instances by priority.
+             */
+            new Comparator<AbstractPureLookup>(){


This should use a lambda.

Rogar that.

jvz · 2020-08-15T19:12:48Z

log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/Interpolator.java

-import java.util.List;
-import java.util.Locale;
-import java.util.Map;
+import java.util.*;


No star imports please!

Rogar that. Get your point! It is an auto action did by IDEA, I've turn up the count. Thanks.

jvz · 2020-08-15T19:12:58Z

log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java

-import org.apache.logging.log4j.core.lookup.MapLookup;
-import org.apache.logging.log4j.core.lookup.StrLookup;
-import org.apache.logging.log4j.core.lookup.StrSubstitutor;
+import org.apache.logging.log4j.core.lookup.*;


No star imports please!

jvz · 2020-08-15T19:15:34Z

log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/AbstractPureLookup.java

+ *
+ * Use to resolve those variables without prefix.
+ *
+ * @author tommy.tesla


Please don't use the @author tag in javadoc.

jvz · 2020-08-15T19:19:47Z

log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/BuiltInLookupCategory.java

+ * @author tommy.testla
+ * @since 3.0.0
+ */
+public interface BuiltInLookupCategory {


Two things about this:

We do not use interfaces for holding string constants; we use a public final class with public static final constants and a private constructor. Using interfaces this way is an anti-pattern.

Due to how you've refactored these to be dynamically looked up via services, these constants are only referenced in their respective classes now. There's no reason to factor out the constants at this point.

Ok, got it.

rgoers · 2020-08-15T23:55:04Z

These changes are not what I expected. All the Lookups should remain plugins. You should have only had to modify Interpolator to load the required lookups via ServiceLoader and then added the Lookups you wanted to load to the file used by ServiceLoader. The Lookups would be loaded again as normal as Plugins during configuration.

tbwork · 2020-08-17T03:40:25Z

@rgoers I propably misunderstood your opinion:

In essence, StrLookup would become the service interface and every plugin to be pre-loaded would be listed in the services file instead of hard coded in Interpolator.

I guess following is what you want:

Load all lookups as plugins via SPI.
Load all lookup plugins via the plugin mechanism.

Is it right ?

tbwork · 2020-08-17T06:56:10Z

@rgoers Hi, rgoers. After debuging and reading the log4j2 codes, I realize that the public Interpolator(final Map<String, String> properties) is called several times during factory creation stage. Is that reasonable to create so many StrLookup instances with the same type?

… prefix

rgoers · 2020-08-17T16:02:06Z

Well, the Interpolator constructor that has the hard-coded list of lookups is initializing the lookups that could be using while constructing the configuration. The other lookups would be used at runtime. There is no reason that I can think of that all the Lookups couldn't be initialized once via ServiceLoader except that it would break compatibility with users who have created their own Lookups as plugins. We could possibly load the serviceloader Lookups once and then add the ones defined as plugins later.

ppkarwasz · 2023-03-01T07:11:43Z

This was closed automatically by Github because we renamed the release-2.x branch to 2.x. Feel free to resubmit to the 2.x branch.

support custom strlookup

9d70ea2

test custom lookup fix support custom lookup

rgoers approved these changes Aug 10, 2020

View reviewed changes

jvz reviewed Aug 10, 2020

View reviewed changes

rgoers requested changes Aug 10, 2020

View reviewed changes

tbwork commented Aug 11, 2020

View reviewed changes

tbwork marked this pull request as draft August 12, 2020 04:07

tbwork changed the title ~~support custom strlookup~~ Reconstruct lookup load mechanism from plugin mode to SPI mode. Aug 12, 2020

tbwork marked this pull request as ready for review August 12, 2020 04:10

jvz reviewed Aug 15, 2020

View reviewed changes

jvz requested changes Aug 15, 2020

View reviewed changes

Keep plugins and load pre-load lookups via SPI

b033210

tbwork force-pushed the support-custom-strlookup branch from d3be33b to b033210 Compare August 17, 2020 05:23

tbwork changed the title ~~Reconstruct lookup load mechanism from plugin mode to SPI mode.~~ Load pre-loaded lookups via SPI, rather than hard-code in Interpolator. Aug 17, 2020

no asterisk in import

9bb343d

support pure lookup which is responsible to resolve variables without…

0e8b8a0

… prefix

tbwork force-pushed the support-custom-strlookup branch from fd6a377 to 0e8b8a0 Compare August 17, 2020 12:42

vy deleted the branch apache:master February 28, 2023 15:02

vy closed this Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load pre-loaded lookups via SPI, rather than hard-code in Interpolator. #396

Load pre-loaded lookups via SPI, rather than hard-code in Interpolator. #396

tbwork commented Aug 10, 2020

tbwork commented Aug 10, 2020

jvz commented Aug 10, 2020

rgoers commented Aug 10, 2020

jvz left a comment

rgoers commented Aug 10, 2020 •

edited

Loading

rgoers left a comment

tbwork commented Aug 10, 2020

tbwork commented Aug 10, 2020

rgoers commented Aug 10, 2020 •

edited

Loading

tbwork commented Aug 11, 2020

tbwork commented Aug 11, 2020

tbwork left a comment

tbwork commented Aug 14, 2020

jvz Aug 15, 2020

tbwork Aug 17, 2020

jvz Aug 15, 2020

jvz Aug 15, 2020

jvz Aug 15, 2020

jvz Aug 15, 2020

jvz Aug 15, 2020

jvz Aug 15, 2020

tbwork Aug 17, 2020

jvz Aug 15, 2020

tbwork Aug 17, 2020 •

edited

Loading

jvz Aug 15, 2020

jvz Aug 15, 2020

jvz Aug 15, 2020

tbwork Aug 17, 2020

rgoers commented Aug 15, 2020

tbwork commented Aug 17, 2020 •

edited

Loading

tbwork commented Aug 17, 2020 •

edited

Loading

rgoers commented Aug 17, 2020

ppkarwasz commented Mar 1, 2023

Load pre-loaded lookups via SPI, rather than hard-code in Interpolator. #396

Load pre-loaded lookups via SPI, rather than hard-code in Interpolator. #396

Conversation

tbwork commented Aug 10, 2020

tbwork commented Aug 10, 2020

jvz commented Aug 10, 2020

rgoers commented Aug 10, 2020

jvz left a comment

Choose a reason for hiding this comment

rgoers commented Aug 10, 2020 • edited Loading

rgoers left a comment

Choose a reason for hiding this comment

tbwork commented Aug 10, 2020

tbwork commented Aug 10, 2020

rgoers commented Aug 10, 2020 • edited Loading

tbwork commented Aug 11, 2020

tbwork commented Aug 11, 2020

tbwork left a comment

Choose a reason for hiding this comment

tbwork commented Aug 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbwork Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgoers commented Aug 15, 2020

tbwork commented Aug 17, 2020 • edited Loading

tbwork commented Aug 17, 2020 • edited Loading

rgoers commented Aug 17, 2020

ppkarwasz commented Mar 1, 2023

rgoers commented Aug 10, 2020 •

edited

Loading

rgoers commented Aug 10, 2020 •

edited

Loading

tbwork Aug 17, 2020 •

edited

Loading

tbwork commented Aug 17, 2020 •

edited

Loading

tbwork commented Aug 17, 2020 •

edited

Loading