Solr
  1. Solr
  2. SOLR-2303

remove unnecessary (and problematic) log4j jars in contribs

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: Build
    • Labels:
      None

      Description

      In solr 4.0 there is log4j-over-slf4j.

      But if you have log4j jars also in the classpath (e.g. contrib/extraction, contrib/clustering) you can get strange errors such as:
      java.lang.NoSuchMethodError: org.apache.log4j.Logger.setAdditivity(Z)V

      So I think we should remove the log4j jars in these contribs, all tests pass with them removed.

      1. SOLR-2303.patch
        0.4 kB
        Robert Muir

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          I think the purpose of the log4j-over-slf4j jars was so that the third party jars included in solr (and in contribs) which use log4j logging will have all of their messages funneled through slf4j so all logging for "basic" solr users will be consistent (JUL) – if you remove it, some solr logging will use slf4j->JUL and some will go direct to log4j.

          I think the other log4j jars you mentioned (contrib/extraction, contrib/clustering) are the ones that should be removed. (untested that this doesn't break anything)

          Show
          Hoss Man added a comment - I think the purpose of the log4j-over-slf4j jars was so that the third party jars included in solr (and in contribs) which use log4j logging will have all of their messages funneled through slf4j so all logging for "basic" solr users will be consistent (JUL) – if you remove it, some solr logging will use slf4j->JUL and some will go direct to log4j. I think the other log4j jars you mentioned (contrib/extraction, contrib/clustering) are the ones that should be removed. (untested that this doesn't break anything)
          Hide
          Robert Muir added a comment -

          hoss, exactly what I tested... I think it doesn't show in the "patch", but I want to remove the log4j jars in the contribs.

          if these are in the classpath, it causes problems for velocity etc (its test will fail). so I think they should be removed from the contribs as it can break functionality in core if you use these contribs (besides just being unnecessary bloat)

          Show
          Robert Muir added a comment - hoss, exactly what I tested... I think it doesn't show in the "patch", but I want to remove the log4j jars in the contribs. if these are in the classpath, it causes problems for velocity etc (its test will fail). so I think they should be removed from the contribs as it can break functionality in core if you use these contribs (besides just being unnecessary bloat)
          Hide
          Hoss Man added a comment -

          I'm an idiot .. trying to catch up on mail i completely missread almost everything about this issue.

          yes, yes .. agree with you 100% .. remove the log4j jars in the contribs

          Show
          Hoss Man added a comment - I'm an idiot .. trying to catch up on mail i completely missread almost everything about this issue. yes, yes .. agree with you 100% .. remove the log4j jars in the contribs
          Hide
          Robert Muir added a comment -

          Also, related to this, the extraction contrib has an old xerces jar (xercesImpl-2.8.1.jar).

          I think this should be upgraded to 2.10, for example the TestXInclude will fail if this is present in the classpath...

          Show
          Robert Muir added a comment - Also, related to this, the extraction contrib has an old xerces jar (xercesImpl-2.8.1.jar). I think this should be upgraded to 2.10, for example the TestXInclude will fail if this is present in the classpath...
          Hide
          Robert Muir added a comment -

          Committed revision 1054405.

          I will open a separate issue for the xerces thing, i noticed other problems (especially in branch-3x).

          I think we should assess all of our external dependencies, as Uwe quotes:

          Uwe: nothing i hate more than downloaded open source projects with antique jars bundled
          
          Show
          Robert Muir added a comment - Committed revision 1054405. I will open a separate issue for the xerces thing, i noticed other problems (especially in branch-3x). I think we should assess all of our external dependencies, as Uwe quotes: Uwe: nothing i hate more than downloaded open source projects with antique jars bundled
          Hide
          Erick Erickson added a comment -

          I am Officially Confused, but the culprit appears to be log4j-over-slf4j-1.5.5.jar

          3_x has:
          log4j jars in solr/contrib/extraction and solr/contrib/clustering
          a bunch of slf4j jars in solr/lib (but NOT log4j-over-slf4j-1.5.5.jar, see below).
          All tests succeed just fine.

          Trunk has:
          no log4j jars in contrib
          the same slf4j jars as in 3_x BUT ALSO log4j-over-slf4j-1.5.5.jar
          VelocityResponseWriterTest fails

          In trunk, removing log4j-over-slf4j-1.5.5.jar allows VelocityResponseWriterTest and all other tests to succeed.

          in 3_x, removing the log4j jars from solr/contrib makes no difference, all tests pass.

          So I propose that the fix for this is to remove the log4j files from 3_x and the log4j-over-slf4j-1.5.5.jar from trunk.

          Should I create a patch? And do patches actually remove jars like this?

          Show
          Erick Erickson added a comment - I am Officially Confused, but the culprit appears to be log4j-over-slf4j-1.5.5.jar 3_x has: log4j jars in solr/contrib/extraction and solr/contrib/clustering a bunch of slf4j jars in solr/lib (but NOT log4j-over-slf4j-1.5.5.jar, see below). All tests succeed just fine. Trunk has: no log4j jars in contrib the same slf4j jars as in 3_x BUT ALSO log4j-over-slf4j-1.5.5.jar VelocityResponseWriterTest fails In trunk, removing log4j-over-slf4j-1.5.5.jar allows VelocityResponseWriterTest and all other tests to succeed. in 3_x, removing the log4j jars from solr/contrib makes no difference, all tests pass. So I propose that the fix for this is to remove the log4j files from 3_x and the log4j-over-slf4j-1.5.5.jar from trunk. Should I create a patch? And do patches actually remove jars like this?
          Hide
          Erick Erickson added a comment -

          See previous comment, I believe that there are some jars in Solr that need to be removed.

          Show
          Erick Erickson added a comment - See previous comment, I believe that there are some jars in Solr that need to be removed.
          Hide
          Robert Muir added a comment -

          Erick, actually i think the issue is that log4j-over-slf4j conflicts with log4j, if log4j is in the classpath.

          The problem is that currently, the solr build runs tests with whatever is in ant's classpath.
          This is why the tests pass for you, even if you remove all logging jars, but this is obviously bad as its not really a repeatable build.

          So to fix this, we need to use includeantruntime=no in the junit tasks, and also not include $java.class.path in the test classpath.
          instead, we explicitly include the ant libs we supply (especially since we extend some of them for testing).

          This might make some warnings or even errors for ant 1.8 users, but I think thats ok.

          Show
          Robert Muir added a comment - Erick, actually i think the issue is that log4j-over-slf4j conflicts with log4j, if log4j is in the classpath. The problem is that currently, the solr build runs tests with whatever is in ant's classpath. This is why the tests pass for you, even if you remove all logging jars, but this is obviously bad as its not really a repeatable build. So to fix this, we need to use includeantruntime=no in the junit tasks, and also not include $java.class.path in the test classpath. instead, we explicitly include the ant libs we supply (especially since we extend some of them for testing). This might make some warnings or even errors for ant 1.8 users, but I think thats ok.
          Hide
          Mark Miller added a comment -

          Hey Erick,

          If I remember right, log4j-over-slf4j is in there for proper zookeeper logging (hoping they switch to slf4j). Rather than dropping it, we should likely try and figure out how to keep and fix the issue - as suggested by Robert.

          Show
          Mark Miller added a comment - Hey Erick, If I remember right, log4j-over-slf4j is in there for proper zookeeper logging (hoping they switch to slf4j). Rather than dropping it, we should likely try and figure out how to keep and fix the issue - as suggested by Robert.
          Hide
          Erick Erickson added a comment -

          Ah, I think the light finally dawns. And helps explain why I'm getting different results on different machines/environments

          There's a reason "they" don't often let me near build systems.....

          Ok, splendid. I suggested removing things to see if it was a bad idea. It is. Almost.

          So does it still make sense to remove the log4j jars in contrib in the 3_x branch?

          Robert:
          I did as you suggested, and of course started getting "classNotFound" errors for JUnitTestRunner and so-on. So I included these lines in Solr's build.xml.

          <pathelement path="$

          {common-solr.dir}/../lucene/lib/ant-junit-1.7.1.jar" />
          <pathelement path="${common-solr.dir}

          /../lucene/lib/ant-1.7.1.jar" />
          <pathelement path="$

          {common-solr.dir}

          /../lucene/lib/junit-4.7.jar" />

          in place of java.class.path and all is well. Is this the path you'd go down? I'm not very comfortable having Solr reach over into Lucene, but what do I know?

          It should be fairly obvious by now that I'm not very ant-sophisticated, is there a preferred way of doing this? Because if this is OK, it seems we should also remove junit-4.7.jar from ../solr/lib and point anything that needs it should path to ../lucene/lib as well.

          I'm currently testing similar changes on the 3_x build with log4j files removed. But that worked before as well.....

          Let me know

          Show
          Erick Erickson added a comment - Ah, I think the light finally dawns. And helps explain why I'm getting different results on different machines/environments There's a reason "they" don't often let me near build systems..... Ok, splendid. I suggested removing things to see if it was a bad idea. It is. Almost. So does it still make sense to remove the log4j jars in contrib in the 3_x branch? Robert: I did as you suggested, and of course started getting "classNotFound" errors for JUnitTestRunner and so-on. So I included these lines in Solr's build.xml. <pathelement path="$ {common-solr.dir}/../lucene/lib/ant-junit-1.7.1.jar" /> <pathelement path="${common-solr.dir} /../lucene/lib/ant-1.7.1.jar" /> <pathelement path="$ {common-solr.dir} /../lucene/lib/junit-4.7.jar" /> in place of java.class.path and all is well. Is this the path you'd go down? I'm not very comfortable having Solr reach over into Lucene, but what do I know? It should be fairly obvious by now that I'm not very ant-sophisticated, is there a preferred way of doing this? Because if this is OK, it seems we should also remove junit-4.7.jar from ../solr/lib and point anything that needs it should path to ../lucene/lib as well. I'm currently testing similar changes on the 3_x build with log4j files removed. But that worked before as well..... Let me know
          Hide
          Erick Erickson added a comment -

          OK, scratch the notion of removing the junit-4.7.jar file from Solr, the test cases...er...stop compiling. But the rest still stands.

          Show
          Erick Erickson added a comment - OK, scratch the notion of removing the junit-4.7.jar file from Solr, the test cases...er...stop compiling. But the rest still stands.
          Hide
          Robert Muir added a comment -

          OK, scratch the notion of removing the junit-4.7.jar file from Solr, the test cases...er...stop compiling. But the rest still stands.

          <pathelement path="$

          Unknown macro: {common-solr.dir}

          /../lucene/lib/ant-junit-1.7.1.jar" />
          <pathelement path="$

          /../lucene/lib/ant-1.7.1.jar" />
          <pathelement path="$

          Unknown macro: {common-solr.dir}

          /../lucene/lib/junit-4.7.jar" />

          in place of java.class.path and all is well. Is this the path you'd go down? I'm not very comfortable having Solr reach over into Lucene, but what do I know?

          Yeah, in general it would be good to explicitly include ant, ant-junit, and junit into our classpath for tests.
          I know i fooled with trying to do this across all of lucene and solr, there are some twists:

          • when the clover build is enabled, we have to actually use the ant runtime/java.class.path, because clover injects itself via ant's classpath via -lib. There
            might be a better way to configure clover to avoid this, but failing that we have to "sometimes" support throwing ant's classpath into the classpath like we do now.
          • the contrib/ant gets tricky (i dont remember why) especially with clover enabled
          • finally, ant 1.8 support might break, since we specifically include ant 1.7 stuff in our lib. But its generally what we want, better to have a reliable classpath in
            our build/tests than to compile/test with "whatever version of ant the person happens to be using". Ant gets angry if you try to put ant 1.7.jar into an ant 1.8 runtime...

          the same situation exists for compilation actually, but I think i fixed that one... you would have to re-check

          Show
          Robert Muir added a comment - OK, scratch the notion of removing the junit-4.7.jar file from Solr, the test cases...er...stop compiling. But the rest still stands. <pathelement path="$ Unknown macro: {common-solr.dir} /../lucene/lib/ant-junit-1.7.1.jar" /> <pathelement path="$ /../lucene/lib/ant-1.7.1.jar" /> <pathelement path="$ Unknown macro: {common-solr.dir} /../lucene/lib/junit-4.7.jar" /> in place of java.class.path and all is well. Is this the path you'd go down? I'm not very comfortable having Solr reach over into Lucene, but what do I know? Yeah, in general it would be good to explicitly include ant, ant-junit, and junit into our classpath for tests. I know i fooled with trying to do this across all of lucene and solr, there are some twists: when the clover build is enabled, we have to actually use the ant runtime/java.class.path, because clover injects itself via ant's classpath via -lib. There might be a better way to configure clover to avoid this, but failing that we have to "sometimes" support throwing ant's classpath into the classpath like we do now. the contrib/ant gets tricky (i dont remember why) especially with clover enabled finally, ant 1.8 support might break, since we specifically include ant 1.7 stuff in our lib. But its generally what we want, better to have a reliable classpath in our build/tests than to compile/test with "whatever version of ant the person happens to be using". Ant gets angry if you try to put ant 1.7.jar into an ant 1.8 runtime... the same situation exists for compilation actually, but I think i fixed that one... you would have to re-check
          Hide
          Erick Erickson added a comment -

          I really can't pursue this farther than I have so far, I may be able to come back to it sometime in the future.

          Show
          Erick Erickson added a comment - I really can't pursue this farther than I have so far, I may be able to come back to it sometime in the future.
          Hide
          Robert Muir added a comment -

          I can work on this, when we decide to move to ant 1.8.1... otherwise I think its too complicated.

          Show
          Robert Muir added a comment - I can work on this, when we decide to move to ant 1.8.1... otherwise I think its too complicated.
          Hide
          Jan Høydahl added a comment -

          Ping() on this before 3.5 - branch_3x still has log4j jars in contrib, can they be removed?

          Show
          Jan Høydahl added a comment - Ping() on this before 3.5 - branch_3x still has log4j jars in contrib, can they be removed?
          Hide
          Robert Muir added a comment -

          I committed the patch to trunk-only before, because at the time it only caused an actual problem on trunk.

          So branch_3x has these extra useless jars, not causing a problem right? But if they are useless, we should nuke them.

          Show
          Robert Muir added a comment - I committed the patch to trunk-only before, because at the time it only caused an actual problem on trunk. So branch_3x has these extra useless jars, not causing a problem right? But if they are useless, we should nuke them.
          Hide
          Uwe Schindler added a comment -

          In all cases!!! And the Maven POMs should also not reference them!

          Uwe-State-Machine

          Show
          Uwe Schindler added a comment - In all cases!!! And the Maven POMs should also not reference them! Uwe-State-Machine
          Hide
          Robert Muir added a comment -

          I merged back to 3.x: contrib/extraction was the only one that had this extra jar there.

          Show
          Robert Muir added a comment - I merged back to 3.x: contrib/extraction was the only one that had this extra jar there.
          Hide
          Uwe Schindler added a comment -

          Bulk close after 3.5 is released

          Show
          Uwe Schindler added a comment - Bulk close after 3.5 is released

            People

            • Assignee:
              Robert Muir
              Reporter:
              Robert Muir
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development