Jetspeed 2
  1. Jetspeed 2
  2. JS2-972

commons-logging unsuited for cross-context webapplication invocation usage - migrating to slf4j

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.2.0
    • Fix Version/s: 2.2.0
    • Component/s: Components Core
    • Labels:
      None

      Description

      This issue is foremost the same as PLUTO-553, so I won't repeat the description and arguments given there.

      However, for Jetspeed the migrating to slf4j requires a bit more changes than merely changing the logger api and maven dependencies fixing.
      Note: over the weekend and today I've already done these locally and tested it successfully on both Tomcat and Websphere (6.1.0.9)

      • Other (third party) dependencies relying on using commons-logging directly
        Examples of these are commons-digester, commons-betwixt and the spring-ojb module.
        For these artifacts, the commons-logging api simply needs to be present at runtime.
        Luckily, slf4j provides the jcl-over-slf4j jar for exactly this purpose: it provides the commons-logging api but will redirect any usage to leverage slf4j under the hoods.
        Jetspeed therefore will have to use this jcl-over-slf4j jar to "fix" these commons-logging usages.
        Note: this obviously requires us to exclude any other dependency on commons-logging in the maven poms to prevent api namespace clashes at runtime.
      • Explicit Commons Logging functionality used within Jetspeed
        The ToolsLogger interface was created to "bridge" Maven Log interface to Commons Logging so that Jetspeed components could be invoked from a Maven plugin while directing their log output through the Maven log.
        Implementations of these ToolsLogger interface, CommonsToolsLogger.java and CommonsLogToolsLogger.java now need to be redefined and reimplemented to "bridge" to SLF4J
      • jetspeed-webapp-logging/apa-webapp-logging (see also: JS2-508)
        The jetspeed-webapp-logging component has been created long time ago to support "isolated" webapplication log management on certain webservers like Websphere and others like JBoss.
        The issue solved by this component was the inability to use both commons-logging and log4j within a webapplication on such webservers properly with as result that log management couldn't be managed and configured locally per webapplication.
        However, by migrating to slf4j (+log4j) this problem is "resolved" automatically, which I've already tested successfully on Websphere (not yet on JBoss).
        The only requirement to get this working is to use a PARENT_LAST (or CHILD_FIRST) webapplication classloader configuration.
        As this is "standard" on Tomcat and for jetspeed required anyway, Jetspeed, nor any other web/portlet application using slf4j, will require the jetspeed-webapp-logging component anymore.
        Therefore, although we already were in the process of moving jetspeed-webapp-logging to Portals Application (apa-webapp-logging), I think we can simply "drop" this component, also from APA, after the slf4j migration!

      I intend to commit the outstanding changes for this migration to slf4j soon, right after the migration for Pluto (PLUTO-553).

        Activity

        Hide
        Ate Douma added a comment -

        slf4j migration for Pluto has been completed (PLUTO-533).
        I'll continue with and commit the migration for Jetspeed, j2-admin and APA tomorrow (no more left time today).

        Show
        Ate Douma added a comment - slf4j migration for Pluto has been completed ( PLUTO-533 ). I'll continue with and commit the migration for Jetspeed, j2-admin and APA tomorrow (no more left time today).
        Hide
        Ate Douma added a comment -

        I've committed the required migration changes for slf4j on both jetspeed-2 and j2-admin now.

        Just to clarify a few of the required changes I didn't describe yet above:

        • slf4j doesn't support FATAL logging.
          We almost had no usages of that, but where they were, I've changed them to ERROR level logging instead.
        • slf4j doesn't support log(Object) but requires (at least) a String as first parameter
          For some of the parameters we passed in this required just a Object.toString(), e.g charSequence.toString() to "fix" it.
          But, we also used log(Exception) in certain locations. For this, slf4j requires changing this to log(String, Exception) and I changed our usage to log(exception.getMessage(), exception).

        What still remains to be "fixed" is the Velocity logging. We used our custom webapp-logging handler for that which I for now changed it to the Velocity provided log class: org.apache.velocity.runtime.log.Log4JLogChute
        However, this somehow doesn't seem to work and produce any log output yet.
        I'll further look into this and try to get this fixed too ASAP.

        Show
        Ate Douma added a comment - I've committed the required migration changes for slf4j on both jetspeed-2 and j2-admin now. Just to clarify a few of the required changes I didn't describe yet above: slf4j doesn't support FATAL logging. We almost had no usages of that, but where they were, I've changed them to ERROR level logging instead. slf4j doesn't support log(Object) but requires (at least) a String as first parameter For some of the parameters we passed in this required just a Object.toString(), e.g charSequence.toString() to "fix" it. But, we also used log(Exception) in certain locations. For this, slf4j requires changing this to log(String, Exception) and I changed our usage to log(exception.getMessage(), exception). What still remains to be "fixed" is the Velocity logging. We used our custom webapp-logging handler for that which I for now changed it to the Velocity provided log class: org.apache.velocity.runtime.log.Log4JLogChute However, this somehow doesn't seem to work and produce any log output yet. I'll further look into this and try to get this fixed too ASAP.
        Hide
        Woonsan Ko added a comment -

        Hi Ate,

        I've just run unit tests today for my JS2-976 task.
        I instantly met an exception complaining commons-logging stuffs.

        -------------------------------------------------------------------------------
        Test set: org.apache.jetspeed.cache.TestContentCache
        -------------------------------------------------------------------------------
        Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.126 sec <<< FAILURE!
        testContentCacheByUser(org.apache.jetspeed.cache.TestContentCache) Time elapsed: 0.086 sec <<< ERROR!
        java.lang.NoClassDefFoundError: org/apache/commons/logging/LogFactory
        at org.springframework.util.ClassUtils.<clinit>(ClassUtils.java:73)
        at org.springframework.core.io.ClassPathResource.<init>(ClassPathResource.java:82)
        at org.springframework.core.io.ClassPathResource.<init>(ClassPathResource.java:64)
        at org.apache.jetspeed.cache.impl.EhCacheConfigResource.afterPropertiesSet(EhCacheConfigResource.java:206)
        at org.apache.jetspeed.cache.impl.EhCacheConfigResource.getInstance(EhCacheConfigResource.java:84)
        at org.apache.jetspeed.cache.TestContentCache.testContentCacheByUser(TestContentCache.java:58)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:585)
        at junit.framework.TestCase.runTest(TestCase.java:154)
        <SNIP>

        I've just investigated the spring source history (ClassUtils.java).
        The ClassUtils.java was changed to not statically use LogFactory since June 4, 2008.
        And, this changes can be applied since Spring Framework 2.5.5 release.

        Therefore, I think it is inevitable to upgrade the version to 2.5.5 or 2.5.6 now.

        If there's no objection to upgrade it to 2.5.6 and no commit before I do, I will do this as soon as possible.

        Show
        Woonsan Ko added a comment - Hi Ate, I've just run unit tests today for my JS2-976 task. I instantly met an exception complaining commons-logging stuffs. ------------------------------------------------------------------------------- Test set: org.apache.jetspeed.cache.TestContentCache ------------------------------------------------------------------------------- Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.126 sec <<< FAILURE! testContentCacheByUser(org.apache.jetspeed.cache.TestContentCache) Time elapsed: 0.086 sec <<< ERROR! java.lang.NoClassDefFoundError: org/apache/commons/logging/LogFactory at org.springframework.util.ClassUtils.<clinit>(ClassUtils.java:73) at org.springframework.core.io.ClassPathResource.<init>(ClassPathResource.java:82) at org.springframework.core.io.ClassPathResource.<init>(ClassPathResource.java:64) at org.apache.jetspeed.cache.impl.EhCacheConfigResource.afterPropertiesSet(EhCacheConfigResource.java:206) at org.apache.jetspeed.cache.impl.EhCacheConfigResource.getInstance(EhCacheConfigResource.java:84) at org.apache.jetspeed.cache.TestContentCache.testContentCacheByUser(TestContentCache.java:58) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at junit.framework.TestCase.runTest(TestCase.java:154) <SNIP> I've just investigated the spring source history (ClassUtils.java). The ClassUtils.java was changed to not statically use LogFactory since June 4, 2008. And, this changes can be applied since Spring Framework 2.5.5 release. Therefore, I think it is inevitable to upgrade the version to 2.5.5 or 2.5.6 now. If there's no objection to upgrade it to 2.5.6 and no commit before I do, I will do this as soon as possible.
        Hide
        Ate Douma added a comment -

        Hi Woonsan,

        We already discussed a bit upgrading to latest Spring 2.5.6 and I think/expect that to be ok to do, so +1 from me.
        Alternatively, to solve these kind of "hard coded" usages of commons logging which also and already have with for instance commons-digester or spring-ojb,
        you can also add a jcl-over-slf4j (test) dependency instead.

        Show
        Ate Douma added a comment - Hi Woonsan, We already discussed a bit upgrading to latest Spring 2.5.6 and I think/expect that to be ok to do, so +1 from me. Alternatively, to solve these kind of "hard coded" usages of commons logging which also and already have with for instance commons-digester or spring-ojb, you can also add a jcl-over-slf4j (test) dependency instead.
        Hide
        Woonsan Ko added a comment -

        Thanks, Ate.
        I think we'd better include jcl-over-slf4j library for runtime because there could be some external jar files dependent on commons-logging, such as commons-digester or spring-ojb as you mentioned. Those are used at runtime.

        Show
        Woonsan Ko added a comment - Thanks, Ate. I think we'd better include jcl-over-slf4j library for runtime because there could be some external jar files dependent on commons-logging, such as commons-digester or spring-ojb as you mentioned. Those are used at runtime.
        Hide
        Ate Douma added a comment -

        I've already added jcl-over-slf4j as dependency on jetspeed-rdbms and jetspeed-db-maven-plugin.
        That dependency on jetspeed-rdbms causes it to be included for the portal at runtime already implicitly but I agree it is better to have it explicitly (probably on the on jetspeed-dependencies).
        The jetspeed-db-maven-plugin also has this dependency as it needs to be run standalone (from the command line).

        Show
        Ate Douma added a comment - I've already added jcl-over-slf4j as dependency on jetspeed-rdbms and jetspeed-db-maven-plugin. That dependency on jetspeed-rdbms causes it to be included for the portal at runtime already implicitly but I agree it is better to have it explicitly (probably on the on jetspeed-dependencies). The jetspeed-db-maven-plugin also has this dependency as it needs to be run standalone (from the command line).
        Hide
        Woonsan Ko added a comment -

        FYI, I met another exception with the unit tests after upgrading spring framework to 2.5.6 locally:

        java.lang.NoClassDefFoundError: org/apache/commons/logging/LogFactory
        at org.springframework.core.io.support.PropertiesLoaderSupport.<init>(PropertiesLoaderSupport.java:46)
        at org.apache.jetspeed.components.JetspeedBeanDefinitionFilter.<init>(JetspeedBeanDefinitionFilter.java:106)
        at org.apache.jetspeed.components.test.AbstractSpringTestCase.getBeanDefinitionFilter(AbstractSpringTestCase.java:81)
        at org.apache.jetspeed.components.test.AbstractSpringTestCase.setUp(AbstractSpringTestCase.java:48)
        at junit.framework.TestCase.runBare(TestCase.java:125)
        <SNIP>

        So, we need jcl-over-slf4j library anyway because spring core itself depends on commons-logging.

        Show
        Woonsan Ko added a comment - FYI, I met another exception with the unit tests after upgrading spring framework to 2.5.6 locally: java.lang.NoClassDefFoundError: org/apache/commons/logging/LogFactory at org.springframework.core.io.support.PropertiesLoaderSupport.<init>(PropertiesLoaderSupport.java:46) at org.apache.jetspeed.components.JetspeedBeanDefinitionFilter.<init>(JetspeedBeanDefinitionFilter.java:106) at org.apache.jetspeed.components.test.AbstractSpringTestCase.getBeanDefinitionFilter(AbstractSpringTestCase.java:81) at org.apache.jetspeed.components.test.AbstractSpringTestCase.setUp(AbstractSpringTestCase.java:48) at junit.framework.TestCase.runBare(TestCase.java:125) <SNIP> So, we need jcl-over-slf4j library anyway because spring core itself depends on commons-logging.
        Hide
        Ate Douma added a comment -

        After I deleted earlier this evening the apa-webapp-logging component I noticed the apa-rss still was using it
        I started cleaning up apa-rss for this and then noticed some other APA component still were not completely "clean" from commons-logging either.
        One thing let to another and I ended up cleaning up all the APA components, including stripping out other no longer needed dependencies, JSP tag lib uri cleanup, etc.

        As a result I've got now most if not all the APA components "clean", not just for commons-logging, I'll take this issue back to commit my changes and thereafter resolve this issue.

        Show
        Ate Douma added a comment - After I deleted earlier this evening the apa-webapp-logging component I noticed the apa-rss still was using it I started cleaning up apa-rss for this and then noticed some other APA component still were not completely "clean" from commons-logging either. One thing let to another and I ended up cleaning up all the APA components, including stripping out other no longer needed dependencies, JSP tag lib uri cleanup, etc. As a result I've got now most if not all the APA components "clean", not just for commons-logging, I'll take this issue back to commit my changes and thereafter resolve this issue.
        Hide
        Ate Douma added a comment -

        All commons-logging dependencies removed, both from jetspeed-2, j2-admin and all APA components.

        Show
        Ate Douma added a comment - All commons-logging dependencies removed, both from jetspeed-2, j2-admin and all APA components.

          People

          • Assignee:
            Ate Douma
            Reporter:
            Ate Douma
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development