Solr
  1. Solr
  2. SOLR-3358

Capture Logging Events from JUL and Log4j

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      The UI should be able to show the last few log messages. To support this, we will need to register an Appender (log4j) or Handler
      (JUL) and keep a buffer of recent log events.

      1. SOLR-3358-logging.patch
        76 kB
        Ryan McKinley
      2. SOLR-3358-logging.patch
        78 kB
        Ryan McKinley
      3. SOLR-3358-compile-path.patch
        2 kB
        Ryan McKinley

        Issue Links

          Activity

          Hide
          Ryan McKinley added a comment -

          This patch moves the LoggerFramework abstraction to its own package and extends it to support registering listeners.

          Some things to note about this patch

          • the log info is static – we don't want multiple instances running for each Core
          • By default it keeps the last 50 error+ messages

          To see the latest messages, call:
          http://localhost:8983/solr/admin/logging?since=0

          To get messages since the last time you asked, pick the last timestamp and call:
          http://localhost:8983/solr/admin/logging?since=1334365940591

          to change the threshold call:
          http://localhost:8983/solr/admin/logging?threshold=ALL

          ------

          This adds log4j to the build classpath but it is not included in the .war

          Show
          Ryan McKinley added a comment - This patch moves the LoggerFramework abstraction to its own package and extends it to support registering listeners. Some things to note about this patch the log info is static – we don't want multiple instances running for each Core By default it keeps the last 50 error+ messages To see the latest messages, call: http://localhost:8983/solr/admin/logging?since=0 To get messages since the last time you asked, pick the last timestamp and call: http://localhost:8983/solr/admin/logging?since=1334365940591 to change the threshold call: http://localhost:8983/solr/admin/logging?threshold=ALL ------ This adds log4j to the build classpath but it is not included in the .war
          Hide
          Stefan Matheis (steffkes) added a comment -

          Ryan, very cool .. what do you think about the following things:

          • combine the key-element which each of the elements in messages
          • split the message element in several parts
          • no specific call for changing the threshold, instead a /admin/logging?level=whatever&since=.. to get all messages for the specified level (and maybe higher ones)
          • possibility to filter the messages by logger

          Right now, i was not able to produce one entry which contains a trace-element, which format will i get there, some plain text which just needs to be shown in the UI?

          Will see if i will be able to bring up a first draft the next days

          Show
          Stefan Matheis (steffkes) added a comment - Ryan, very cool .. what do you think about the following things: combine the key -element which each of the elements in messages split the message element in several parts no specific call for changing the threshold, instead a /admin/logging?level=whatever&since=.. to get all messages for the specified level (and maybe higher ones) possibility to filter the messages by logger Right now, i was not able to produce one entry which contains a trace -element, which format will i get there, some plain text which just needs to be shown in the UI? Will see if i will be able to bring up a first draft the next days
          Hide
          Ryan McKinley added a comment -

          After thinking a bit more on this, I think we can (optionally) take it another step forward. The log appender/handler could actually send the event to a solr index. Then we could support more complex search/filtering directly with solr.

          As a first step, I'll replace my custom format with a SolrDocumentList so that anything we do for the in-memory solution could be reused for an index based solution.

          In a cloud/distributed environment, if all the instances log to the same server it would be much easier to spot problems across the whole system.

          I'll work up a patch

          Right now, i was not able to produce one entry which contains a trace-element

          try:
          http://localhost:8983/solr/admin/logging?since=0&test=true
          this will add a message (and exception) at each level

          Show
          Ryan McKinley added a comment - After thinking a bit more on this, I think we can (optionally) take it another step forward. The log appender/handler could actually send the event to a solr index. Then we could support more complex search/filtering directly with solr. As a first step, I'll replace my custom format with a SolrDocumentList so that anything we do for the in-memory solution could be reused for an index based solution. In a cloud/distributed environment, if all the instances log to the same server it would be much easier to spot problems across the whole system. I'll work up a patch Right now, i was not able to produce one entry which contains a trace-element try: http://localhost:8983/solr/admin/logging?since=0&test=true this will add a message (and exception) at each level
          Hide
          Ryan McKinley added a comment -

          Here is an updated patch.

          Rather then have a static initialization in a RequestHandler, this moves the LogWatcher to CoreContainer and makes sure it is initialized early in the process.

          This also avoids wrapping each event like the previous patch. Each framework needs to convert the message (LogRecord or LoggingEvent) to a SolrDocument that gets passed to the UI

          Show
          Ryan McKinley added a comment - Here is an updated patch. Rather then have a static initialization in a RequestHandler, this moves the LogWatcher to CoreContainer and makes sure it is initialized early in the process. This also avoids wrapping each event like the previous patch. Each framework needs to convert the message (LogRecord or LoggingEvent) to a SolrDocument that gets passed to the UI
          Hide
          Ryan McKinley added a comment -

          Added key infrastructure in r1327210.

          I will make new issues for ongoing work

          Show
          Ryan McKinley added a comment - Added key infrastructure in r1327210. I will make new issues for ongoing work
          Hide
          Chris Male added a comment - - edited

          Hey Ryan,

          My IntelliJ is complaining about compiling Log4jWatcher. I think its because of the classpath collision of the log4j and log4j-over-slf4j jars. Both include a org.apache.log4j.LogManager and org.apache.log4j.spi.LoggingEvent, yet they aren't the same classes. Any thoughts here? I'm not sure we can have the jars as they are.

          Show
          Chris Male added a comment - - edited Hey Ryan, My IntelliJ is complaining about compiling Log4jWatcher. I think its because of the classpath collision of the log4j and log4j-over-slf4j jars. Both include a org.apache.log4j.LogManager and org.apache.log4j.spi.LoggingEvent , yet they aren't the same classes. Any thoughts here? I'm not sure we can have the jars as they are.
          Hide
          Steve Rowe added a comment -

          Ryan, Maven compile is failing, AFAICT as a result of your commit: https://builds.apache.org/job/Lucene-Solr-Maven-trunk/459/consoleText

          Show
          Steve Rowe added a comment - Ryan, Maven compile is failing, AFAICT as a result of your commit: https://builds.apache.org/job/Lucene-Solr-Maven-trunk/459/consoleText
          Hide
          Steve Rowe added a comment -

          Ryan, Maven compile is failing, AFAICT as a result of your commit: https://builds.apache.org/job/Lucene-Solr-Maven-trunk/459/consoleText

          I just committed a fix: from solr-core POM, exclude log4j-over-slf4j transitive dependency from solrj dependency. Compile/test works locally for me.

          Show
          Steve Rowe added a comment - Ryan, Maven compile is failing, AFAICT as a result of your commit: https://builds.apache.org/job/Lucene-Solr-Maven-trunk/459/consoleText I just committed a fix: from solr-core POM, exclude log4j-over-slf4j transitive dependency from solrj dependency. Compile/test works locally for me.
          Hide
          Steve Rowe added a comment -

          My IntelliJ is complaining about compiling Log4jWatcher. I think its because of the classpath collision of the log4j and log4j-over-slf4j jars. Both include a org.apache.log4j.LogManager and org.apache.log4j.spi.LoggingEvent, yet they aren't the same classes. Any thoughts here? I'm not sure we can have the jars as they are.

          Ant compilation succeeds only because log4j is ordered before log4j-over-slf4j in the classpath. This is bad.

          Show
          Steve Rowe added a comment - My IntelliJ is complaining about compiling Log4jWatcher. I think its because of the classpath collision of the log4j and log4j-over-slf4j jars. Both include a org.apache.log4j.LogManager and org.apache.log4j.spi.LoggingEvent, yet they aren't the same classes. Any thoughts here? I'm not sure we can have the jars as they are. Ant compilation succeeds only because log4j is ordered before log4j-over-slf4j in the classpath. This is bad.
          Hide
          Ryan McKinley added a comment -

          thanks steven

          I'm looking at the pom issue now. Ideally the over* will be optional dependencies in solrj, excluded from solr-core and included in the .war (this lets the end user decide what logging framework they actually use)

          For the ant build path, any suggestions? we can exclude it from core, and then add it back to the .war file. I'm not sure how to get the ivy stuff setup to have .war dependencies though.

          Show
          Ryan McKinley added a comment - thanks steven I'm looking at the pom issue now. Ideally the over * will be optional dependencies in solrj, excluded from solr-core and included in the .war (this lets the end user decide what logging framework they actually use) For the ant build path, any suggestions? we can exclude it from core, and then add it back to the .war file. I'm not sure how to get the ivy stuff setup to have .war dependencies though.
          Hide
          Steve Rowe added a comment -

          Reopening to fix Solr compilation classpath to exclude log4j-over-slf4j

          Show
          Steve Rowe added a comment - Reopening to fix Solr compilation classpath to exclude log4j-over-slf4j
          Hide
          Steve Rowe added a comment -

          For the ant build path, any suggestions?

          All solr/lib/ jars are put on the compilation classpath via the additional.dependencies path. I added log4j-over-slf4j to the exclude list and compile/test for all solr+contribs succeeds:

            <path id="additional.dependencies">
          -   <fileset dir="${common-solr.dir}/lib" excludes="${common.classpath.excludes}"/>
          +   <fileset dir="${common-solr.dir}/lib"
          +            excludes="${common.classpath.excludes},log4j-over-slf4j*"/>
              <fileset dir="${common-solr.dir}/example/lib" excludes="${common.classpath.excludes}"/>
              <fileset dir="lib" excludes="${common.classpath.excludes}" erroronmissingdir="false"/>
            </path>
          

          we can exclude it from core, and then add it back to the .war file. I'm not sure how to get the ivy stuff setup to have .war dependencies though.

          ant dist under solr/webapp/, which produces the .war, doesn't use additional.dependencies - instead it uses common.classpath.excludes. So I think with the above change, log4j-over-slf4j will continue to be included in the .war.

          Show
          Steve Rowe added a comment - For the ant build path, any suggestions? All solr/lib/ jars are put on the compilation classpath via the additional.dependencies path. I added log4j-over-slf4j to the exclude list and compile/test for all solr+contribs succeeds: <path id= "additional.dependencies" > - <fileset dir= "${common-solr.dir}/lib" excludes= "${common.classpath.excludes}" /> + <fileset dir= "${common-solr.dir}/lib" + excludes= "${common.classpath.excludes},log4j-over-slf4j*" /> <fileset dir= "${common-solr.dir}/example/lib" excludes= "${common.classpath.excludes}" /> <fileset dir= "lib" excludes= "${common.classpath.excludes}" erroronmissingdir= "false" /> </path> we can exclude it from core, and then add it back to the .war file. I'm not sure how to get the ivy stuff setup to have .war dependencies though. ant dist under solr/webapp/ , which produces the .war, doesn't use additional.dependencies - instead it uses common.classpath.excludes . So I think with the above change, log4j-over-slf4j will continue to be included in the .war.
          Hide
          Steve Rowe added a comment - - edited

          So I think with the above change, log4j-over-slf4j will continue to be included in the .war.

          Confirmed - after running ant dist from solr/webapp/ with the above change in solr/common-build.xml, I can see from running jar tvf solr/dist/*.war:

          ...
          481535 Wed Mar 31 00:25:34 EDT 2010 WEB-INF/lib/log4j-1.2.16.jar
           20639 Mon Oct 31 18:46:50 EDT 2011 WEB-INF/lib/log4j-over-slf4j-1.6.4.jar
          ...
          

          But as you can see, log4j is still there - is that intentional, Ryan?

          Show
          Steve Rowe added a comment - - edited So I think with the above change, log4j-over-slf4j will continue to be included in the .war. Confirmed - after running ant dist from solr/webapp/ with the above change in solr/common-build.xml , I can see from running jar tvf solr/dist/*.war : ... 481535 Wed Mar 31 00:25:34 EDT 2010 WEB-INF/lib/log4j-1.2.16.jar 20639 Mon Oct 31 18:46:50 EDT 2011 WEB-INF/lib/log4j-over-slf4j-1.6.4.jar ... But as you can see, log4j is still there - is that intentional, Ryan?
          Hide
          Robert Muir added a comment -

          hmm, 4 logging jars, excluding certain jars for different things here and there,
          this creates a lot of complexity and dependency management.

          log4j-over-slf4j already has the log4j api, it must for it to work (http://www.slf4j.org/legacy.html)

          so I don't understand why the log4j jar is being added! Why is this necessary?
          If we are going to add log4j, then slf4j should removed,
          otherwise the whole purpose of a logging facade is defeated.

          One of these must go.

          Show
          Robert Muir added a comment - hmm, 4 logging jars, excluding certain jars for different things here and there, this creates a lot of complexity and dependency management. log4j-over-slf4j already has the log4j api, it must for it to work ( http://www.slf4j.org/legacy.html ) so I don't understand why the log4j jar is being added! Why is this necessary? If we are going to add log4j, then slf4j should removed, otherwise the whole purpose of a logging facade is defeated. One of these must go.
          Hide
          Ryan McKinley added a comment -

          Steven, do these .pom changes make sense to you?

          +1 on your additional.dependencies change

          The problem with idea build is that it includes everything in lib. The options I see are to either list the jars explicitly (leaving out -over-slf4j) or make a new lib folder under webapp. Thoughts?

          Show
          Ryan McKinley added a comment - Steven, do these .pom changes make sense to you? +1 on your additional.dependencies change The problem with idea build is that it includes everything in lib. The options I see are to either list the jars explicitly (leaving out -over-slf4j) or make a new lib folder under webapp. Thoughts?
          Hide
          Steve Rowe added a comment -

          Steven, do these .pom changes make sense to you?

          Yes, but there is a syntax issue - all of these:

          <scope>optional</scope>
          

          should instead be:

          <optional>true</optional>
          
          Show
          Steve Rowe added a comment - Steven, do these .pom changes make sense to you? Yes, but there is a syntax issue - all of these: <scope> optional </scope> should instead be: <optional> true </optional>
          Hide
          Ryan McKinley added a comment -

          so I don't understand why the log4j jar is being added! Why is this necessary?

          The log4j-over-slf4j.jar only wraps the calls to write events, it does not wrap anything for handling events (Appender and LoggingEvent). Ideally log4j would only be in the classpath for:
          https://svn.apache.org/repos/asf/lucene/dev/trunk/solr/core/src/java/org/apache/solr/logging/log4j/

          The point of this is to capture events if the end user binds JUL or Log4j. Alternatively I could put the log4j implementation elsewhere, but that seems kinda silly since lots of people actually use log4j

          Show
          Ryan McKinley added a comment - so I don't understand why the log4j jar is being added! Why is this necessary? The log4j-over-slf4j.jar only wraps the calls to write events, it does not wrap anything for handling events (Appender and LoggingEvent). Ideally log4j would only be in the classpath for: https://svn.apache.org/repos/asf/lucene/dev/trunk/solr/core/src/java/org/apache/solr/logging/log4j/ The point of this is to capture events if the end user binds JUL or Log4j. Alternatively I could put the log4j implementation elsewhere, but that seems kinda silly since lots of people actually use log4j
          Hide
          Uwe Schindler added a comment -

          Ryan: But you have now a classpath conflict. Depending on the order of jar files in filesystem and order of jar files resulting by that undefinedness, it can happen that log4j events are no longer transferred to slf4j (if user does not use log4j).

          The log4j-over-slf is just a "catcher" for compatibility with external libs bundeled with Solr, that use log4j as their logging framework (it emulates log4j). The user has to sort out what chains he need to get correct logging, means: slf4 main jar, the delegator to log4j and log4j to do the actual work, or alternatively log4j-over-slf4j (to catch 3rd party logging events) and sl4fj (like it was before), that will log to JUL or nowhere. Theoretically maybe commons-logging cather is also needed for some 3rd party libs like commons-digester.

          I would like that we revert this commit and discuss the logging again! This will be a pain for all WAR users and is already wrong because of duplicate class files in classpath leading to bugs or maybe even stack overflows because of logging-loops (I have seen that already, one logging framework delegates to another one and finally to itsself).

          Show
          Uwe Schindler added a comment - Ryan: But you have now a classpath conflict. Depending on the order of jar files in filesystem and order of jar files resulting by that undefinedness, it can happen that log4j events are no longer transferred to slf4j (if user does not use log4j). The log4j-over-slf is just a "catcher" for compatibility with external libs bundeled with Solr, that use log4j as their logging framework (it emulates log4j). The user has to sort out what chains he need to get correct logging, means: slf4 main jar, the delegator to log4j and log4j to do the actual work, or alternatively log4j-over-slf4j (to catch 3rd party logging events) and sl4fj (like it was before), that will log to JUL or nowhere. Theoretically maybe commons-logging cather is also needed for some 3rd party libs like commons-digester. I would like that we revert this commit and discuss the logging again! This will be a pain for all WAR users and is already wrong because of duplicate class files in classpath leading to bugs or maybe even stack overflows because of logging-loops (I have seen that already, one logging framework delegates to another one and finally to itsself).
          Hide
          Steve Rowe added a comment -

          The problem with idea build is that it includes everything in lib. The options I see are to either list the jars explicitly (leaving out -over-slf4j) or make a new lib folder under webapp. Thoughts?

          I'd rather not list the jars explicitly - IntelliJ dependency configuration has been quite stable because whole directories are specified, and I'd rather not change that if possible.

          So my vote is to make a new lib folder under webapp.

          (I can confirm, though, that explicitly listing all jars except log4j-over-slf4j allows compilation and testing to succeed.)

          Show
          Steve Rowe added a comment - The problem with idea build is that it includes everything in lib. The options I see are to either list the jars explicitly (leaving out -over-slf4j) or make a new lib folder under webapp. Thoughts? I'd rather not list the jars explicitly - IntelliJ dependency configuration has been quite stable because whole directories are specified, and I'd rather not change that if possible. So my vote is to make a new lib folder under webapp. (I can confirm, though, that explicitly listing all jars except log4j-over-slf4j allows compilation and testing to succeed.)
          Hide
          Ryan McKinley added a comment -

          I would like that we revert this commit and discuss the logging again!

          I'll pull out the log4j implementation, but leave in the JUL one – is that OK for now? or take out the whole thing?

          Show
          Ryan McKinley added a comment - I would like that we revert this commit and discuss the logging again! I'll pull out the log4j implementation, but leave in the JUL one – is that OK for now? or take out the whole thing?
          Hide
          Ryan McKinley added a comment -

          log4j implementation pulled out in r1327608

          Let me know if you want me to pull out the whole thing...

          This will be a pain for all WAR users

          Note the .war does not include log4j.jar

          The aim is to provide a log4j implementation that can be used if the end user implements logging with log4j.

          Again, I am happy to put this elsewhere, but given that log4j is pretty common it would be nice to make it work out-of-the-box

          The first question is if we want to support log4j out of the box.

          If so, I think the right approach is to add a new lib folder to webapp and put all the slf4j-over .jar files in there

          Show
          Ryan McKinley added a comment - log4j implementation pulled out in r1327608 Let me know if you want me to pull out the whole thing... This will be a pain for all WAR users Note the .war does not include log4j.jar The aim is to provide a log4j implementation that can be used if the end user implements logging with log4j. Again, I am happy to put this elsewhere, but given that log4j is pretty common it would be nice to make it work out-of-the-box The first question is if we want to support log4j out of the box. If so, I think the right approach is to add a new lib folder to webapp and put all the slf4j-over .jar files in there
          Hide
          Robert Muir added a comment -

          I'm gonna dodge your question about 'do we support logging framework X' because i don't really
          care about logging, i just piped up because i care about the build or release packaging being
          complicated.

          The issue stumbles upon an existing problem actually: the solr dependencies and compilation classpaths
          are already quite confusing:

          • solr tests suck in huge classpaths from solr/lib and example/lib and other places: not really
            testing the 'desired classpath'. For example, nobody wants solrj to depend on lucene-core or
            lucene-spellchecker, but i can easily add new SpellChecker() to solrj java files and all tests pass.
          • the lack of separation (e.g core/lib and solrj/lib), makes packaging a mystery: i dont even know
            what packaging magic makes the solrj-lib in the binary distribution. But whatever it is probably
            error-prone (SOLR-3374)
          • the webapp/ is confusing to me: if solr core doesn't actually depend on jetty and can even work
            embedded, then why does it have classes that include/extend jetty/servlet classes? Shouldn't all
            this stuff be in webapp/ to cleanly separate it out?
          • the example/ in my opinion should also be treated as another 'module' and the 'example tests' should
            sit underneath it. I think its confusing how other tests reach around to the example.

          I think these are generally unrelated to your patch: but trying to do what you want with logging jars
          just puts it over the edge in complexity.

          If things like alternative logging frameworks and servlet containers are actually intended to be supported,
          then shouldn't we:

          1. fix all these classpaths to be per-module, enforcing that our dependencies are what we think they are,
            and that we package up what we think we are packaging up.
          2. add things like alternative ivy configurations to allow us to test these different implementations
            (e.g. log4j, tomcat, etc) at least in hudson via -D switches, otherwise how do we know they actually work
            without manual testing?
          Show
          Robert Muir added a comment - I'm gonna dodge your question about 'do we support logging framework X' because i don't really care about logging, i just piped up because i care about the build or release packaging being complicated. The issue stumbles upon an existing problem actually: the solr dependencies and compilation classpaths are already quite confusing: solr tests suck in huge classpaths from solr/lib and example/lib and other places: not really testing the 'desired classpath'. For example, nobody wants solrj to depend on lucene-core or lucene-spellchecker, but i can easily add new SpellChecker() to solrj java files and all tests pass. the lack of separation (e.g core/lib and solrj/lib), makes packaging a mystery: i dont even know what packaging magic makes the solrj-lib in the binary distribution. But whatever it is probably error-prone ( SOLR-3374 ) the webapp/ is confusing to me: if solr core doesn't actually depend on jetty and can even work embedded, then why does it have classes that include/extend jetty/servlet classes? Shouldn't all this stuff be in webapp/ to cleanly separate it out? the example/ in my opinion should also be treated as another 'module' and the 'example tests' should sit underneath it. I think its confusing how other tests reach around to the example. I think these are generally unrelated to your patch: but trying to do what you want with logging jars just puts it over the edge in complexity. If things like alternative logging frameworks and servlet containers are actually intended to be supported, then shouldn't we: fix all these classpaths to be per-module, enforcing that our dependencies are what we think they are, and that we package up what we think we are packaging up. add things like alternative ivy configurations to allow us to test these different implementations (e.g. log4j, tomcat, etc) at least in hudson via -D switches, otherwise how do we know they actually work without manual testing?
          Hide
          Uwe Schindler added a comment -

          Hi,

          I strongly agree here with Robert, one thing to add: Maven (sorry) has the notion of "runtime", "test" and "compile" (and even "test-compile") classpaths, which may all be different. Log4J is not the only problem that we have on that, since the upgrade to Jetty 8 we have a serious flaw here, too: I just repeat that on every issue because it seems nobody takes care or maybe nobody understands the problem: The Solr webapp / Solr core should be compatible with a lot of servlet containers. All on-the-market stable servlet containers are build on servlet-api 2.4 or 2.5. For compiling Solr-webapp, this API is enough and we must test and compile against and only against servlet-api-2.4 (like we did with Java 5 in LuSolr 3.x). If we then run tests with Jetty 8, we must of course plug in the Jetty-provided servlet-api (which is 3.0), but for compile we must use the generic original servlet-api-2.4 interfaces JAR file from Sun Microsystems (not a Jetty or Tomcat fake).

          For compiling Solr Core, we should only have the slf4j-api.jar file in classpath, no impls or delegators! No log4j, no commons-logging. To compile those "extra addons loaded by reflection", we can use a separate compilation unit only containing the appender/listerners for various logging systems compiled solely against its own oficially provided JAR file (not e.g. log4j-over-sfl4j.jar, whcih is a fake like mentioned above).

          My proposal:

          • We should separate the lib folder to handle compile time-only references and runtime/tests execution. The runtime classpatch is also packaged into WAR file.
          • As Robert suggests: More clear separation into modules, so Solr core does not need jetty/servlet classes to compile or even run!
          • Only use original interface JARs on the minimum required version (servlet 2.4), from the official provider (servlet-api 2.4 from Sun Microsystems, log4j from Apache,...) while compiling (like using Java 5 rt.jar in Lucene 3.x), to ensure compatibility with public APIs.
          Show
          Uwe Schindler added a comment - Hi, I strongly agree here with Robert, one thing to add: Maven (sorry) has the notion of "runtime", "test" and "compile" (and even "test-compile") classpaths, which may all be different. Log4J is not the only problem that we have on that, since the upgrade to Jetty 8 we have a serious flaw here, too: I just repeat that on every issue because it seems nobody takes care or maybe nobody understands the problem: The Solr webapp / Solr core should be compatible with a lot of servlet containers. All on-the-market stable servlet containers are build on servlet-api 2.4 or 2.5. For compiling Solr-webapp, this API is enough and we must test and compile against and only against servlet-api-2.4 (like we did with Java 5 in LuSolr 3.x). If we then run tests with Jetty 8, we must of course plug in the Jetty-provided servlet-api (which is 3.0), but for compile we must use the generic original servlet-api-2.4 interfaces JAR file from Sun Microsystems (not a Jetty or Tomcat fake). For compiling Solr Core , we should only have the slf4j-api.jar file in classpath, no impls or delegators! No log4j, no commons-logging. To compile those "extra addons loaded by reflection", we can use a separate compilation unit only containing the appender/listerners for various logging systems compiled solely against its own oficially provided JAR file (not e.g. log4j-over-sfl4j.jar, whcih is a fake like mentioned above). My proposal: We should separate the lib folder to handle compile time-only references and runtime/tests execution. The runtime classpatch is also packaged into WAR file. As Robert suggests: More clear separation into modules, so Solr core does not need jetty/servlet classes to compile or even run! Only use original interface JARs on the minimum required version (servlet 2.4), from the official provider (servlet-api 2.4 from Sun Microsystems, log4j from Apache,...) while compiling (like using Java 5 rt.jar in Lucene 3.x), to ensure compatibility with public APIs.
          Hide
          Robert Muir added a comment -

          I strongly agree here with Robert, one thing to add: Maven (sorry) has the notion of "runtime", "test" and "compile" (and even "test-compile") classpaths, which may all be different.

          The parallel exists in ivy, its easy to use. its called configurations (we actually already use these), and you can
          name them whatever you want.

          with such configurations we could even have things like test-classpath-jetty, test-classpath-tomcat, test-classpath-log4j, whatever whatever.

          We already use configurations (for a different purpose) in some of the solr ivy.xml's...

          If we wanted, i'm sure such a thing could be used to even e.g. implement lucene's test-backwards and things like that
          in the future. This is really a similar situation in many ways if you think about it...

          Show
          Robert Muir added a comment - I strongly agree here with Robert, one thing to add: Maven (sorry) has the notion of "runtime", "test" and "compile" (and even "test-compile") classpaths, which may all be different. The parallel exists in ivy, its easy to use. its called configurations (we actually already use these), and you can name them whatever you want. with such configurations we could even have things like test-classpath-jetty, test-classpath-tomcat, test-classpath-log4j, whatever whatever. We already use configurations (for a different purpose) in some of the solr ivy.xml's... If we wanted, i'm sure such a thing could be used to even e.g. implement lucene's test-backwards and things like that in the future. This is really a similar situation in many ways if you think about it...
          Hide
          Ryan McKinley added a comment -

          yes, solr classpath is a disaster! I agree with everything you say/suggest, i figured this was the tradeoff for avoiding maven

          The other thing to consider is a test classpath – the only class that uses jetty in the src tree is JettySolrRunner. I think that could be moved to test.

          re SpellChecker in solrj. The maven build should fail if you do this since it does use different classpaths for each module. (not a real solution, but just pointing it out)

          As a first step, I think we should remove the single solr/lib folder and replace it with:
          solrj/lib
          core/lib
          core/lib-test (includes jetty)
          webapp/lib (has the slf4j-over stuff)

          Show
          Ryan McKinley added a comment - yes, solr classpath is a disaster! I agree with everything you say/suggest, i figured this was the tradeoff for avoiding maven The other thing to consider is a test classpath – the only class that uses jetty in the src tree is JettySolrRunner. I think that could be moved to test. re SpellChecker in solrj. The maven build should fail if you do this since it does use different classpaths for each module. (not a real solution, but just pointing it out) As a first step, I think we should remove the single solr/lib folder and replace it with: solrj/lib core/lib core/lib-test (includes jetty) webapp/lib (has the slf4j-over stuff)
          Hide
          Ryan McKinley added a comment -

          For compiling Solr-webapp, this API is enough and we must test and compile against and only against servlet-api-2.4

          FWIW, the maven build uses 2.4 so we would get a jenkins failure if we use something 3.0 specific

          if we had different lib/classpath for core/lib-test and example/lib we could use jetty-7 in core/lib-test and jetty-8 in example/lib – this would let us test both jetty 7 and 8

          Show
          Ryan McKinley added a comment - For compiling Solr-webapp, this API is enough and we must test and compile against and only against servlet-api-2.4 FWIW, the maven build uses 2.4 so we would get a jenkins failure if we use something 3.0 specific if we had different lib/classpath for core/lib-test and example/lib we could use jetty-7 in core/lib-test and jetty-8 in example/lib – this would let us test both jetty 7 and 8
          Hide
          Yonik Seeley added a comment -

          Trying to support different servlet containers, different logging frameworks, etc, is a mess... IMO, we should be considering Solr more as a server (and the fact that it uses Jetty and Java logging an implementation detail). I don't think making everything pluggable buys us much but a lot of headaches.

          Show
          Yonik Seeley added a comment - Trying to support different servlet containers, different logging frameworks, etc, is a mess... IMO, we should be considering Solr more as a server (and the fact that it uses Jetty and Java logging an implementation detail). I don't think making everything pluggable buys us much but a lot of headaches.
          Hide
          Robert Muir added a comment -

          Well for one the current structure is really unrelated to maven. When Steve Rowe and I
          first started working on the build, there were other things to fix (core tests depended on contribs, etc).

          So i think its improving, I'm not trying to complain: if we could have fixed this stuff
          safely when transitioning to ivy, we would have. I feel like Chris Male and I spent an entire
          night trying to figure out the best approach: populating the huge lib/ folder just like before
          simple kept the issue scope contained.

          Just a few notes about this:

          1. we don't really need to be downloading jars and putting them in lib/ at all (LUCENE-3943).
            what we have now is a 'transition mechanism' that works like the old build, but we should
            really fix this: then people wont have issues with things like having stale jars or anything
            else: and we avoid copying around or whatever. Still lets put this aside, we can still improve
            things right now (see below)
          2. i agree with your suggestions, though i think we actually have jetty-test classes in test-framework?
            so i think it should be solrj/lib, core/lib, test-framework/lib, etc (don't try to tackle the lib-test
            initially, i think we should attack 'separation' first).
          3. after accomplishing #2 above, we can then start to think about alternative configurations,
            and whether or not we even want to try to do that before fixing #1. Fixing #1 would make this task
            much simpler and cleaner. i don't think we should have a separate lib/ with jetty7 and example with jetty8,
            because i think we would ultimately want the flexibility to be able to run both core and example tests with
            any of (jetty6, jetty7, jetty8, tomcat-xx, ...), and this could be specified with a -D or something.
            Perhaps the defaults are set to something like you suggest, but hudson could test other possibilities.
          Show
          Robert Muir added a comment - Well for one the current structure is really unrelated to maven. When Steve Rowe and I first started working on the build, there were other things to fix (core tests depended on contribs, etc). So i think its improving, I'm not trying to complain: if we could have fixed this stuff safely when transitioning to ivy, we would have. I feel like Chris Male and I spent an entire night trying to figure out the best approach: populating the huge lib/ folder just like before simple kept the issue scope contained. Just a few notes about this: we don't really need to be downloading jars and putting them in lib/ at all ( LUCENE-3943 ). what we have now is a 'transition mechanism' that works like the old build, but we should really fix this: then people wont have issues with things like having stale jars or anything else: and we avoid copying around or whatever. Still lets put this aside, we can still improve things right now (see below) i agree with your suggestions, though i think we actually have jetty-test classes in test-framework? so i think it should be solrj/lib, core/lib, test-framework/lib, etc (don't try to tackle the lib-test initially, i think we should attack 'separation' first). after accomplishing #2 above, we can then start to think about alternative configurations, and whether or not we even want to try to do that before fixing #1. Fixing #1 would make this task much simpler and cleaner. i don't think we should have a separate lib/ with jetty7 and example with jetty8, because i think we would ultimately want the flexibility to be able to run both core and example tests with any of (jetty6, jetty7, jetty8, tomcat-xx, ...), and this could be specified with a -D or something. Perhaps the defaults are set to something like you suggest, but hudson could test other possibilities.
          Hide
          Mark Miller added a comment -

          Yup - by having nothing out of the box and trying to support everything you just ensure you support everything poorly. We should pick a framework and give good out of the box logging. We are a search server first, not a lib. I think our current logging situation is in the stone age.

          Show
          Mark Miller added a comment - Yup - by having nothing out of the box and trying to support everything you just ensure you support everything poorly. We should pick a framework and give good out of the box logging. We are a search server first, not a lib. I think our current logging situation is in the stone age.
          Hide
          Robert Muir added a comment -

          Regardless of how you feel about whether or not we support the different containers,
          whether solr should be modular or monolithic, the classpath for solrj etc is still totally wrong.

          You can make it depend on anything in solr/lib (e.g. as i said put a SpellChecker or IndexWriter
          in a solrj class), and all tests pass. In fact, I can go find at least 2 situations (one caused
          by me, the other by merging SolrCloud) where this actually happened that solrj depended on lucene.

          Only manual inspection finds this out, but tests should really fail.

          Besides the lack of no enforcement that solrj should only depend on certain things, there is no simple
          solrj/lib folder for packaging solrj-lib (instead some error-prone inclusions/exclusions)

          Show
          Robert Muir added a comment - Regardless of how you feel about whether or not we support the different containers, whether solr should be modular or monolithic, the classpath for solrj etc is still totally wrong. You can make it depend on anything in solr/lib (e.g. as i said put a SpellChecker or IndexWriter in a solrj class), and all tests pass. In fact, I can go find at least 2 situations (one caused by me, the other by merging SolrCloud) where this actually happened that solrj depended on lucene. Only manual inspection finds this out, but tests should really fail. Besides the lack of no enforcement that solrj should only depend on certain things, there is no simple solrj/lib folder for packaging solrj-lib (instead some error-prone inclusions/exclusions)
          Hide
          Chris Male added a comment -

          +1 to the improvements of the solr classpaths and being wary of over pluggability.

          I think our current logging situation is in the stone age.

          Are you able to elaborate on what improvements you think should happen?

          Show
          Chris Male added a comment - +1 to the improvements of the solr classpaths and being wary of over pluggability. I think our current logging situation is in the stone age. Are you able to elaborate on what improvements you think should happen?
          Hide
          Mark Miller added a comment -

          log4j implementation pulled out in r1327608

          I don't think this went right? It seems borked - now if Log4j is detected, it tries to load a class called Log4j which does not exist...

          Show
          Mark Miller added a comment - log4j implementation pulled out in r1327608 I don't think this went right? It seems borked - now if Log4j is detected, it tries to load a class called Log4j which does not exist...
          Hide
          Ryan McKinley added a comment -

          check revision 1331522

          This now logs an warning rather then throwing an exception when logging can not be initialized.

          does that fix thigns for you?

          Show
          Ryan McKinley added a comment - check revision 1331522 This now logs an warning rather then throwing an exception when logging can not be initialized. does that fix thigns for you?
          Show
          Antony Stubbs added a comment - As per https://issues.apache.org/jira/browse/SOLR-3367?focusedCommentId=13417331&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13417331 , does anyone have a setup already for logback?
          Hide
          Lance Norskog added a comment -

          What if instead you add a collection for logs? I think this would just need a new slf4j implementation that uses solrj. We would not be forced to pick a logging package.

          Show
          Lance Norskog added a comment - What if instead you add a collection for logs? I think this would just need a new slf4j implementation that uses solrj. We would not be forced to pick a logging package.
          Hide
          Ryan McKinley added a comment -

          Closing this issue to be included in SOLR-3706

          Show
          Ryan McKinley added a comment - Closing this issue to be included in SOLR-3706

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Ryan McKinley
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development