Mahout
  1. Mahout
  2. MAHOUT-361

SLF4J dependency structure leads to unpleasant surproses

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.3
    • Fix Version/s: 0.4
    • Component/s: Clustering
    • Labels:
      None

      Description

      Our poms declare a dependency on the slf4j core, but not on any of the implementation modules. Thus, if an unsuspecting user adds a dependency on our stuff, and runs, they get a exception from slf4j complaining that there's no implementation. I claim that it would be more better to declare a dependency on the JDK14 module, and those users who really care about using something else can exclude it and include their own.

        Activity

        Hide
        Sean Owen added a comment -

        Yah in theory it's their project that should depend on an slf4j implementation, not Mahout. But that represents a gotcha, and, if you are right that their dependency will override (does it? because then two bindings are on the classpath? or if properly overridden, their "job" jars won't have our dependency?) then it's good. I like.

        Show
        Sean Owen added a comment - Yah in theory it's their project that should depend on an slf4j implementation, not Mahout. But that represents a gotcha, and, if you are right that their dependency will override (does it? because then two bindings are on the classpath? or if properly overridden, their "job" jars won't have our dependency?) then it's good. I like.
        Hide
        Drew Farris added a comment -

        I've run into this too as a result of having the slf4j implementation in test scope.

        Can we consider continuing to use the jcl adapter instead of jdk14?

        A large number of Mahout's dependenices already use jcl for their logging. Using this adapter changing the logging behavior for mahout (and its dependencies) becomes as simple as adding log4j (or not) to the jars (as opposed to switching the jcl version of slf4j >and< adding log4j). Admittedly this a minor difference, but one less thing to change. The default behavior is to use jdk14 anyway.

        I propose that we remove the test scope from the slf4j-jcl adapter dependency and let it default to the compile scope. Benson, this should solve the issue too, correct?

        Show
        Drew Farris added a comment - I've run into this too as a result of having the slf4j implementation in test scope. Can we consider continuing to use the jcl adapter instead of jdk14? A large number of Mahout's dependenices already use jcl for their logging. Using this adapter changing the logging behavior for mahout (and its dependencies) becomes as simple as adding log4j (or not) to the jars (as opposed to switching the jcl version of slf4j >and< adding log4j). Admittedly this a minor difference, but one less thing to change. The default behavior is to use jdk14 anyway. I propose that we remove the test scope from the slf4j-jcl adapter dependency and let it default to the compile scope. Benson, this should solve the issue too, correct?
        Hide
        Benson Margulies added a comment -

        It's a little worse than 'override'. They must write a Maven 'exclude' clause into their dependency on Mahout, and then write a dependency for the back-end they want.

        This is perhaps why some people are shy of using SLF4J as their direct logging API.

        As per Drew, we could just make calls to JCL, and anyone who wants SLF4J (including us in tests or utilities) can use their bridge.

        Actually, I meant to write jcl, not jdk14, so that proposal is fine with me.

        Show
        Benson Margulies added a comment - It's a little worse than 'override'. They must write a Maven 'exclude' clause into their dependency on Mahout, and then write a dependency for the back-end they want. This is perhaps why some people are shy of using SLF4J as their direct logging API. As per Drew, we could just make calls to JCL, and anyone who wants SLF4J (including us in tests or utilities) can use their bridge. Actually, I meant to write jcl, not jdk14, so that proposal is fine with me.
        Hide
        Sean Owen added a comment -

        Hoo boy, and we used to use JCL directly in 0.1. We come full circle.

        I still somehow favor sticking with SLF4J, mostly from inertia. But I dont' feel strongly.

        But if we're dropping the meta-meta logger, why only drop down to meta-logger JCL? go all the way to JDK's logger. Hey, we already living in a situation where every major logger is used. Some of them have ways to redirect logging to other loggers. We can just use the most-primitive logger and let those who care redirect to JDK's logging?

        Show
        Sean Owen added a comment - Hoo boy, and we used to use JCL directly in 0.1. We come full circle. I still somehow favor sticking with SLF4J, mostly from inertia. But I dont' feel strongly. But if we're dropping the meta-meta logger, why only drop down to meta-logger JCL? go all the way to JDK's logger. Hey, we already living in a situation where every major logger is used. Some of them have ways to redirect logging to other loggers. We can just use the most-primitive logger and let those who care redirect to JDK's logging?
        Hide
        Drew Farris added a comment -

        Sorry, I probably wasn't being clear; I'm not proposing we switch to the jcl (commons-logging) api. I think we should continue to use the slf4j api and continue to use the slf4j-jcl adapter. I do not think we should switch to use the jcl logging api.

        So, it sounds like we are all in agreement, the only change that would need to be made to the existing core pom would be to remove the test scope for the slf4j-jcl adapter dependency. I'm unsure if there are issues with the other poms. Not in the position to look at this moment.

        Show
        Drew Farris added a comment - Sorry, I probably wasn't being clear; I'm not proposing we switch to the jcl (commons-logging) api. I think we should continue to use the slf4j api and continue to use the slf4j-jcl adapter. I do not think we should switch to use the jcl logging api. So, it sounds like we are all in agreement, the only change that would need to be made to the existing core pom would be to remove the test scope for the slf4j-jcl adapter dependency. I'm unsure if there are issues with the other poms. Not in the position to look at this moment.
        Hide
        Benson Margulies added a comment - - edited

        Keep in mind that I got unhappy about this because some really not-very-loggy classes in collections now touch the slf4j API and demand a working config.

        I see two plausible alternatives.

        Alternative #1: remove the test scope. This requires non-JCL users to put some relatively fiddly stuff in their POM, but it avoids requiring everyone to pick one and add it. And the result works out-of-the-box. And for some people, jcl is good enough in terms of logging flexibility.

        Alternative #2: rethink this.

        2a: use java.util logging. Anyone who wants another backend can use the SLF4J technology.
        2b: Borrow from CXF. CXF wanted to (a) support I18N and (b) avoid the situation we are in right now. So, they wrote their own simple logging wrapper. By default, it goes to j.u.l, but by putting a file in classpath you tell it to use some other logger.

        In short, they built a mini-SLF4J with two big differences: I18N and a working default configuration. Since that code is sitting in a nearby Apache project, we can pretty easily expropriate it.

        Show
        Benson Margulies added a comment - - edited Keep in mind that I got unhappy about this because some really not-very-loggy classes in collections now touch the slf4j API and demand a working config. I see two plausible alternatives. Alternative #1: remove the test scope. This requires non-JCL users to put some relatively fiddly stuff in their POM, but it avoids requiring everyone to pick one and add it. And the result works out-of-the-box. And for some people, jcl is good enough in terms of logging flexibility. Alternative #2: rethink this. 2a: use java.util logging. Anyone who wants another backend can use the SLF4J technology. 2b: Borrow from CXF. CXF wanted to (a) support I18N and (b) avoid the situation we are in right now. So, they wrote their own simple logging wrapper. By default, it goes to j.u.l, but by putting a file in classpath you tell it to use some other logger. In short, they built a mini-SLF4J with two big differences: I18N and a working default configuration. Since that code is sitting in a nearby Apache project, we can pretty easily expropriate it.
        Hide
        Sean Owen added a comment -

        How about removing logging statements from collections and math? seems too low level for logging, really.

        Show
        Sean Owen added a comment - How about removing logging statements from collections and math? seems too low level for logging, really.
        Hide
        Benson Margulies added a comment -

        Well, OK, but if assembling a typical job still requires the user to pick and include an slf4j backend, I submit that it's not a great thing.

        Show
        Benson Margulies added a comment - Well, OK, but if assembling a typical job still requires the user to pick and include an slf4j backend, I submit that it's not a great thing.
        Hide
        Ceki Gulcu added a comment -

        FYI, it is likely that future SLF4J release will default to a nop implementation if no binding can be found, eliminating the need to bundle a binding that the user will potentially have to exclude.

        Show
        Ceki Gulcu added a comment - FYI, it is likely that future SLF4J release will default to a nop implementation if no binding can be found, eliminating the need to bundle a binding that the user will potentially have to exclude.
        Hide
        Sean Owen added a comment -

        This is what it looks like to remove logging from math and collections. It's something to consider separately from the main conversation. Since the module in question was math/collections, maybe this is a helpful step anyway. Fewer dependencies in core libraries are better. But does this lose anything valuable? I sense this is left over from Colt?

        Show
        Sean Owen added a comment - This is what it looks like to remove logging from math and collections. It's something to consider separately from the main conversation. Since the module in question was math/collections, maybe this is a helpful step anyway. Fewer dependencies in core libraries are better. But does this lose anything valuable? I sense this is left over from Colt?
        Hide
        Benson Margulies added a comment -

        Ceki, noop would be not as nice as j.u.l.

        Show
        Benson Margulies added a comment - Ceki, noop would be not as nice as j.u.l.
        Hide
        Benson Margulies added a comment -

        That lone collections business is pretty silly, and removing it looks good. I'm not qualified to pass on the implications of de-logging some of the more complex math bits.

        Show
        Benson Margulies added a comment - That lone collections business is pretty silly, and removing it looks good. I'm not qualified to pass on the implications of de-logging some of the more complex math bits.
        Hide
        Sean Owen added a comment -

        Jake I kind of recall you didn't want these log statements removed. Am I right? If so, won't commit my attached patch which just removes collections/math logging.

        But in any event looks like slf4j 1.6 is out which I believe defaults to a no-op implementation, so using that ought to rectify the original issue.

        Show
        Sean Owen added a comment - Jake I kind of recall you didn't want these log statements removed. Am I right? If so, won't commit my attached patch which just removes collections/math logging. But in any event looks like slf4j 1.6 is out which I believe defaults to a no-op implementation, so using that ought to rectify the original issue.
        Hide
        Sean Owen added a comment -

        I didn't remove anything, just updated to slf4j 1.6 which should no-op by default.

        Show
        Sean Owen added a comment - I didn't remove anything, just updated to slf4j 1.6 which should no-op by default.
        Hide
        Hudson added a comment -

        Integrated in Mahout-Quality #32 (See http://hudson.zones.apache.org/hudson/job/Mahout-Quality/32/)
        MAHOUT-361 – just update to slf4j 1.6

        Show
        Hudson added a comment - Integrated in Mahout-Quality #32 (See http://hudson.zones.apache.org/hudson/job/Mahout-Quality/32/ ) MAHOUT-361 – just update to slf4j 1.6

          People

          • Assignee:
            Sean Owen
            Reporter:
            Benson Margulies
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development