Solr
  1. Solr
  2. SOLR-4977

info stream in solrconfig should have option for writing to the solr log

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      Having a separate file is annoying, plus the print stream option doesn't rollover on size or date, doesn't have custom formatting options, etc. Exactly what the logging lib is meant to handle.

      1. SOLR-4977.patch
        8 kB
        Ryan Ernst
      2. SOLR-4977.patch
        8 kB
        Ryan Ernst
      3. SOLR-4977.patch
        8 kB
        Ryan Ernst
      4. SOLR-4977.patch
        7 kB
        Ryan Ernst
      5. SOLR-4977.patch
        6 kB
        Ryan Ernst
      6. SOLR-4977.patch
        6 kB
        Ryan Ernst
      7. SOLR-4977.patch
        6 kB
        Ryan Ernst

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          +1, this would be nice.

          Show
          Mark Miller added a comment - +1, this would be nice.
          Hide
          Ryan Ernst added a comment -

          This patch adds a LoggingInfoStream in solr which just passes the message to slf4j. It is at info level for now; adding support for other log levels can happen later in a separate jira.

          Show
          Ryan Ernst added a comment - This patch adds a LoggingInfoStream in solr which just passes the message to slf4j. It is at info level for now; adding support for other log levels can happen later in a separate jira.
          Hide
          Robert Muir added a comment -

          Looks pretty good: two comments:

          • This might be useful for developers to use in tests actually: The problem is during testing (when assertions are enabled), it can get flooded with "testpoints". So what I did in lucene was to exclude component="TP" (means test point). I think it would be good to do here too.
          • Do you think we should deprecate the /infostream/file method? Like we can issue a warning if someone does this, because really they could configure this guy to go to its own file (without rolling or whatever) via their logging configuration instead?
          Show
          Robert Muir added a comment - Looks pretty good: two comments: This might be useful for developers to use in tests actually: The problem is during testing (when assertions are enabled), it can get flooded with "testpoints". So what I did in lucene was to exclude component="TP" (means test point). I think it would be good to do here too. Do you think we should deprecate the /infostream/file method? Like we can issue a warning if someone does this, because really they could configure this guy to go to its own file (without rolling or whatever) via their logging configuration instead?
          Hide
          Ryan Ernst added a comment -

          New patch addressing Robert's comments.

          Show
          Ryan Ernst added a comment - New patch addressing Robert's comments.
          Hide
          Robert Muir added a comment -

          my only other comment is i think it should go to slf4j with LoggerFactory.getLogger instead of log4j?
          (yes, its confusing log4j is in the classpath...)

          Show
          Robert Muir added a comment - my only other comment is i think it should go to slf4j with LoggerFactory.getLogger instead of log4j? (yes, its confusing log4j is in the classpath...)
          Hide
          Robert Muir added a comment -

          And we probably want to fix the verbage/examples under example/**/solrconfig.xml to make it obvious it goes to logging and remove the deprecated file example...

          Show
          Robert Muir added a comment - And we probably want to fix the verbage/examples under example/**/solrconfig.xml to make it obvious it goes to logging and remove the deprecated file example...
          Hide
          Ryan Ernst added a comment -

          my only other comment is i think it should go to slf4j with LoggerFactory.getLogger instead of log4j?

          Doh! I blame autocomplete. Fixed.

          And we probably want to fix the verbage/examples under example/**/solrconfig.xml

          Good point. Done.

          New patch addresses both of these.

          Also, here is an example of the output (using junit's log format):

          14690 T10 C0 oasu.LoggingInfoStream.message [IW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: now flush at close waitForMerges=true
          14711 T10 C0 oasu.LoggingInfoStream.message [IW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]:   start flush: applyAllDeletes=true
          14712 T10 C0 oasu.LoggingInfoStream.message [IW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]:   index before flush
          14712 T10 C0 oasu.LoggingInfoStream.message [DW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker startFullFlush
          14712 T10 C0 oasu.LoggingInfoStream.message [DW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: anyChanges? numDocsInRam=0 deletes=false hasTickets:false pendingChangesInFullFlush: false
          
          Show
          Ryan Ernst added a comment - my only other comment is i think it should go to slf4j with LoggerFactory.getLogger instead of log4j? Doh! I blame autocomplete. Fixed. And we probably want to fix the verbage/examples under example/**/solrconfig.xml Good point. Done. New patch addresses both of these. Also, here is an example of the output (using junit's log format): 14690 T10 C0 oasu.LoggingInfoStream.message [IW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: now flush at close waitForMerges=true 14711 T10 C0 oasu.LoggingInfoStream.message [IW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: start flush: applyAllDeletes=true 14712 T10 C0 oasu.LoggingInfoStream.message [IW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: index before flush 14712 T10 C0 oasu.LoggingInfoStream.message [DW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker startFullFlush 14712 T10 C0 oasu.LoggingInfoStream.message [DW][SUITE-TestIndexingPerformance-seed#[F536639DD826197C]-worker]: anyChanges? numDocsInRam=0 deletes=false hasTickets:false pendingChangesInFullFlush: false
          Hide
          Ryan Ernst added a comment -

          Attempting to upload the patch again to get the newer version...

          Show
          Ryan Ernst added a comment - Attempting to upload the patch again to get the newer version...
          Hide
          Ryan Ernst added a comment -

          And one more with the hdfs example config file fixed.

          Show
          Ryan Ernst added a comment - And one more with the hdfs example config file fixed.
          Hide
          Robert Muir added a comment -

          patch looks good Ryan. One last question:

          Should we change isEnabled to look like this?

            @Override
            public boolean isEnabled(String component) {
              // ignore testpoints so this can be used with tests without flooding logs with verbose messages
              return !"TP".equals(component) && log.isInfoEnabled();
            }
          

          This way IndexWriter doesnt calculate anything if its not configured to log. We could even consider "enabling" this in solrconfig.xml by default, and instead deferring the control via the logging system. So we'd turn off logging for LoggingInfoStream in logging.properties by default, but someone could then turn it on there, or on-the-fly via the admin gui?

          Show
          Robert Muir added a comment - patch looks good Ryan. One last question: Should we change isEnabled to look like this? @Override public boolean isEnabled( String component) { // ignore testpoints so this can be used with tests without flooding logs with verbose messages return ! "TP" .equals(component) && log.isInfoEnabled(); } This way IndexWriter doesnt calculate anything if its not configured to log. We could even consider "enabling" this in solrconfig.xml by default, and instead deferring the control via the logging system. So we'd turn off logging for LoggingInfoStream in logging.properties by default, but someone could then turn it on there, or on-the-fly via the admin gui?
          Hide
          Ryan Ernst added a comment -

          Great idea!

          This new patch has this implemented. I tested with the solr example and was able to switch the infostream logging on and off with the admin UI!

          Show
          Ryan Ernst added a comment - Great idea! This new patch has this implemented. I tested with the solr example and was able to switch the infostream logging on and off with the admin UI!
          Hide
          ASF subversion and git services added a comment -

          Commit 1498936 from Robert Muir
          [ https://svn.apache.org/r1498936 ]

          SOLR-4977: Add option to send infostream to the logging system

          Show
          ASF subversion and git services added a comment - Commit 1498936 from Robert Muir [ https://svn.apache.org/r1498936 ] SOLR-4977 : Add option to send infostream to the logging system
          Hide
          ASF subversion and git services added a comment -

          Commit 1498944 from Robert Muir
          [ https://svn.apache.org/r1498944 ]

          SOLR-4977: Add option to send infostream to the logging system

          Show
          ASF subversion and git services added a comment - Commit 1498944 from Robert Muir [ https://svn.apache.org/r1498944 ] SOLR-4977 : Add option to send infostream to the logging system
          Hide
          ASF subversion and git services added a comment -

          Commit 1498956 from Robert Muir
          [ https://svn.apache.org/r1498956 ]

          SOLR-4977: remove deprecated option

          Show
          ASF subversion and git services added a comment - Commit 1498956 from Robert Muir [ https://svn.apache.org/r1498956 ] SOLR-4977 : remove deprecated option
          Hide
          ASF subversion and git services added a comment -

          Commit 1498957 from Robert Muir
          [ https://svn.apache.org/r1498957 ]

          SOLR-4977: add back this null check, getConfig calls ensureOpen...

          Show
          ASF subversion and git services added a comment - Commit 1498957 from Robert Muir [ https://svn.apache.org/r1498957 ] SOLR-4977 : add back this null check, getConfig calls ensureOpen...
          Hide
          ASF subversion and git services added a comment -

          Commit 1498958 from Robert Muir
          [ https://svn.apache.org/r1498958 ]

          SOLR-4977: add back this null check, getConfig calls ensureOpen...

          Show
          ASF subversion and git services added a comment - Commit 1498958 from Robert Muir [ https://svn.apache.org/r1498958 ] SOLR-4977 : add back this null check, getConfig calls ensureOpen...
          Hide
          Robert Muir added a comment -

          Thanks Ryan!

          Show
          Robert Muir added a comment - Thanks Ryan!
          Hide
          Steve Rowe added a comment -

          Bulk close resolved 4.4 issues

          Show
          Steve Rowe added a comment - Bulk close resolved 4.4 issues
          Hide
          Shikhar Bhushan added a comment -

          LoggingInfoStream does not log the core name which is an important piece of context - created SOLR-5505 for this

          Show
          Shikhar Bhushan added a comment - LoggingInfoStream does not log the core name which is an important piece of context - created SOLR-5505 for this

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development