Details

    • Type: Wish Wish
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.4
    • Component/s: None
    • Labels:
      None

      Description

      After lots of discussion, we should consider using SLF4j to enable more flexibility in logging configuration.

      See:
      http://www.nabble.com/Solr-Logging-td16836646.html
      http://www.nabble.com/logging-through-log4j-td13747253.html

      1. slf4j-api-1.5.0.jar
        16 kB
        Ryan McKinley
      2. slf4j-jdk14-1.5.0.jar
        8 kB
        Ryan McKinley
      3. SOLR-560-slf4j.patch
        104 kB
        Ryan McKinley
      4. SOLR-560-slf4j.patch
        116 kB
        Will Johnson
      5. SOLR-560-slf4j.patch
        68 kB
        Ryan McKinley
      6. SOLR-560-slf4j.patch
        66 kB
        Ryan McKinley

        Issue Links

          Activity

          Hide
          Grant Ingersoll added a comment -

          AIUI, the LICENSE file needs to contain all licenses. NOTICES needs to contain notices, and not licenses. I don't think it needs to be duplicated.

          Show
          Grant Ingersoll added a comment - AIUI, the LICENSE file needs to contain all licenses. NOTICES needs to contain notices, and not licenses. I don't think it needs to be duplicated.
          Hide
          Ryan McKinley added a comment -

          added license to LICENSE.txt

          so we should have duplicate info in NOTICE.txt and LICENSE.txt? or should the license in NOTICE.txt be removed?

          http://svn.apache.org/viewvc/lucene/solr/trunk/NOTICE.txt?r1=696539&r2=696538&pathrev=696539

          Show
          Ryan McKinley added a comment - added license to LICENSE.txt so we should have duplicate info in NOTICE.txt and LICENSE.txt? or should the license in NOTICE.txt be removed? http://svn.apache.org/viewvc/lucene/solr/trunk/NOTICE.txt?r1=696539&r2=696538&pathrev=696539
          Hide
          Grant Ingersoll added a comment -

          You need to add in the license information to the license file. http://www.slf4j.org/license.html

          See solr/lib/README.committers.txt

          Show
          Grant Ingersoll added a comment - You need to add in the license information to the license file. http://www.slf4j.org/license.html See solr/lib/README.committers.txt
          Hide
          Ryan McKinley added a comment -

          i'll close this for now... if we run into any issues we can re-open it or make a new issue

          Show
          Ryan McKinley added a comment - i'll close this for now... if we run into any issues we can re-open it or make a new issue
          Hide
          Yonik Seeley added a comment -

          Now that we are into 1.4, I would like to commit this soon...

          +1

          Show
          Yonik Seeley added a comment - Now that we are into 1.4, I would like to commit this soon... +1
          Hide
          Ryan McKinley added a comment -

          updated to apply with latest code.

          Now that we are into 1.4, I would like to commit this soon...

          Show
          Ryan McKinley added a comment - updated to apply with latest code. Now that we are into 1.4, I would like to commit this soon...
          Hide
          Ryan McKinley added a comment -

          I removed this from the 1.3 list...

          Hopefully we can add it just after the 1.3 release so we have plenty of time to know if it is an ok choice.

          Show
          Ryan McKinley added a comment - I removed this from the 1.3 list... Hopefully we can add it just after the 1.3 release so we have plenty of time to know if it is an ok choice.
          Hide
          Mike Klaas added a comment -

          It's Ryan's patch.

          Show
          Mike Klaas added a comment - It's Ryan's patch.
          Hide
          David Smiley added a comment -

          I've been using this with JUL and Log4j just fine. I made tweaks to the JSP so that it would compile. Applying the patch didn't work for me because it had references to unexpanded $Id$ in the source yet my checkout had them expanded. So I did a replace-all regexp on all the source to unexpand these down to just $Id$ and then I could apply the patch.

          Show
          David Smiley added a comment - I've been using this with JUL and Log4j just fine. I made tweaks to the JSP so that it would compile. Applying the patch didn't work for me because it had references to unexpanded $Id$ in the source yet my checkout had them expanded. So I did a replace-all regexp on all the source to unexpand these down to just $Id$ and then I could apply the patch.
          Hide
          Will Johnson added a comment -

          patch updated for the latest trunk. i also tested that it works with slf4j redirecting to log4j.

          Show
          Will Johnson added a comment - patch updated for the latest trunk. i also tested that it works with slf4j redirecting to log4j.
          Hide
          Sean Timm added a comment -

          Thanks for taking a look at SOLR-554, Patrick.

          I did test the log level selector with this SLF4J patch and it works fine as is. It might be desirable to change the log level names in the log level selector to match the names in SLF4J however.

          From the SLF4J FAQ:
          "SLF4J is only a facade, meaning that it does not provide a complete logging solution. Operations such as [...] setting logging levels cannot be performed with SLF4J."

          Show
          Sean Timm added a comment - Thanks for taking a look at SOLR-554 , Patrick. I did test the log level selector with this SLF4J patch and it works fine as is. It might be desirable to change the log level names in the log level selector to match the names in SLF4J however. From the SLF4J FAQ: "SLF4J is only a facade, meaning that it does not provide a complete logging solution. Operations such as [...] setting logging levels cannot be performed with SLF4J."
          Hide
          patrick o'leary added a comment -

          I'd suggest replacing logging.jsp with SOLR-554 might need to modify it slightly for SLF4J.

          Show
          patrick o'leary added a comment - I'd suggest replacing logging.jsp with SOLR-554 might need to modify it slightly for SLF4J.
          Hide
          Ryan McKinley added a comment -

          removes unused java.util.logging.Level import.

          Also NOTE, this patch does not handle logging.jsp yet...

          Show
          Ryan McKinley added a comment - removes unused java.util.logging.Level import. Also NOTE, this patch does not handle logging.jsp yet...
          Hide
          Ryan McKinley added a comment -

          I attached a quick patch to convert to SLF4j. I have not fully tested to make sure the default behavior is the same as it was before, but a quick look at the example seems reasonable.

          This changed:
          log.finest -> log.trace
          log.fine -> log.debug
          log.warning -> log.warn
          log.severe -> log.error
          (info stays the same)

          The only bit I was not sure how to translate is in SolrRequestParsers.java:

            log.throwing(getClass().getName(), "getTransformer", tce);
          

          for now, i just used:

           log.error( getClass().getName() + " getTransformer", tce );
          

          Additionally, wherever I noticed it, I used slf4j style "formatters":
          http://www.slf4j.org/faq.html#logging_performance
          but we may want to make a more through review

          Show
          Ryan McKinley added a comment - I attached a quick patch to convert to SLF4j. I have not fully tested to make sure the default behavior is the same as it was before, but a quick look at the example seems reasonable. This changed: log.finest -> log.trace log.fine -> log.debug log.warning -> log.warn log.severe -> log.error (info stays the same) The only bit I was not sure how to translate is in SolrRequestParsers.java: log.throwing(getClass().getName(), "getTransformer" , tce); for now, i just used: log.error( getClass().getName() + " getTransformer" , tce ); Additionally, wherever I noticed it, I used slf4j style "formatters": http://www.slf4j.org/faq.html#logging_performance but we may want to make a more through review

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Ryan McKinley
            • Votes:
              3 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development