Solr
  1. Solr
  2. SOLR-8324 Logger Untanglement
  3. SOLR-8330

Restrict logger visibility throughout the codebase to private so that only the file that declares it can use it

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 6.0
    • Fix Version/s: 5.4, 6.0
    • Component/s: None
    • Labels:

      Description

      As Mike Drob pointed out in Solr-8324, many loggers in Solr are unintentionally shared between classes. Many instances of this are caused by overzealous copy-paste. This can make debugging tougher, as messages appear to come from an incorrect location.

      As discussed in the comments on SOLR-8324, there also might be legitimate reasons for sharing loggers between classes. Where any ambiguity exists, these instances shouldn't be touched.

      1. SOLR-8330.patch
        528 kB
        Anshum Gupta
      2. SOLR-8330.patch
        514 kB
        Jason Gerlowski
      3. SOLR-8330.patch
        485 kB
        Anshum Gupta
      4. SOLR-8330.patch
        423 kB
        Anshum Gupta
      5. SOLR-8330.patch
        394 kB
        Jason Gerlowski
      6. SOLR-8330.patch
        394 kB
        Jason Gerlowski
      7. SOLR-8330.patch
        393 kB
        Jason Gerlowski
      8. SOLR-8330-combined.patch
        507 kB
        Jason Gerlowski
      9. SOLR-8330-detector.patch
        1 kB
        Uwe Schindler
      10. SOLR-8330-detector.patch
        1 kB
        Uwe Schindler

        Activity

        Hide
        Jason Gerlowski added a comment -

        I'm going to take an initial crack at this. Though I can't think of any reasonable rationale for re-using loggers between classes, there might be a use case I'm not aware of. So where there's any doubt, I'll be leaving those declarations as-is. (This can always be changed later if there's a stronger consensus on being stricter about this.)

        For the logger declarations that I do change, I plan on using the copy-paste safe declaration suggested by Uwe on SOLR-8324:

            import java.lang.invoke.MethodHandles;
            //...
            private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
        
        Show
        Jason Gerlowski added a comment - I'm going to take an initial crack at this. Though I can't think of any reasonable rationale for re-using loggers between classes, there might be a use case I'm not aware of. So where there's any doubt, I'll be leaving those declarations as-is. (This can always be changed later if there's a stronger consensus on being stricter about this.) For the logger declarations that I do change, I plan on using the copy-paste safe declaration suggested by Uwe on SOLR-8324 : import java.lang.invoke.MethodHandles; //... private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
        Hide
        Mark Miller added a comment -

        Though I can't think of any reasonable rationale for re-using loggers between classes, there might be a use case I'm not aware of.

        If a dev really wants to make logging more confusing by making something look like another class is logging it, it really should be on them to comment appropriately at these places to prevent other devs from fixing it.

        Show
        Mark Miller added a comment - Though I can't think of any reasonable rationale for re-using loggers between classes, there might be a use case I'm not aware of. If a dev really wants to make logging more confusing by making something look like another class is logging it, it really should be on them to comment appropriately at these places to prevent other devs from fixing it.
        Hide
        Jason Gerlowski added a comment -

        Personally, I agree with you. I was just trying to avoid being overzealous. I saw Yonik and some others had expressed a bit of doubt around this on the original JIRA, so I was being conservative.

        But I'm happy to take a broader approach here and remove all logger-sharing. Done.

        Show
        Jason Gerlowski added a comment - Personally, I agree with you. I was just trying to avoid being overzealous. I saw Yonik and some others had expressed a bit of doubt around this on the original JIRA, so I was being conservative. But I'm happy to take a broader approach here and remove all logger-sharing. Done.
        Hide
        Yonik Seeley added a comment -

        Please let's not blindly convert all loggers.
        As an example, TransactionLog - the inner classes use the outer classes logger, but that's a good thing here IMO - they are highly coupled.

        Show
        Yonik Seeley added a comment - Please let's not blindly convert all loggers. As an example, TransactionLog - the inner classes use the outer classes logger, but that's a good thing here IMO - they are highly coupled.
        Hide
        Jason Gerlowski added a comment - - edited

        I'm fine w/ either approach, I don't have a strong opinion either way (as you can probably tell by my earlier flip-flop).

        But just for clarification, what's the advantage of having TransactionLog on a message that comes from SubclassOfTLog. Under either approach, someone investigating that message should be able to easily find it in any modern IDE. Is it that TransactionLog is a more well-known piece of Solr than an internal impl class, so readers would have a better notion of what's going on than if the message was tagged with a lesser known class?

        Anyways, I can avoid touching the particular outer class-logger-used in-inner-class scenarios exemplified by TransactionLog are there any other cases I should be leaving as-is? I'm not sure how to infer intent vs. accident/laziness in other scenarios unless there's an explicit comment clarifying things, which I don't really expect there to be in many cases.

        If I don't hear anything else in the next day I'll go forward based on the feedback you guys've given so far.

        Show
        Jason Gerlowski added a comment - - edited I'm fine w/ either approach, I don't have a strong opinion either way (as you can probably tell by my earlier flip-flop). But just for clarification, what's the advantage of having TransactionLog on a message that comes from SubclassOfTLog . Under either approach, someone investigating that message should be able to easily find it in any modern IDE. Is it that TransactionLog is a more well-known piece of Solr than an internal impl class, so readers would have a better notion of what's going on than if the message was tagged with a lesser known class? Anyways, I can avoid touching the particular outer class-logger-used in-inner-class scenarios exemplified by TransactionLog are there any other cases I should be leaving as-is? I'm not sure how to infer intent vs. accident/laziness in other scenarios unless there's an explicit comment clarifying things, which I don't really expect there to be in many cases. If I don't hear anything else in the next day I'll go forward based on the feedback you guys've given so far.
        Hide
        Mark Miller added a comment -

        We should simply comment those to protect them if there is no agreement. Personally, I want the logs to point me to the correct class.

        Show
        Mark Miller added a comment - We should simply comment those to protect them if there is no agreement. Personally, I want the logs to point me to the correct class.
        Hide
        Shawn Heisey added a comment -

        We should simply comment those to protect them if there is no agreement. Personally, I want the logs to point me to the correct class.

        I'm with Mark on this. Someone who has no interest in the code won't know what it means, but if they send the logs to someone who does have that interest (including the mailing list or a Jira issue), then logging a different class name will slow down efforts to help.

        I can't think of a situation where a shared logger is actually a good idea, but that might be a failure of imagination on my part. If there IS a good reason to do it, that reason should definitely be in a comment on the logger creation.

        Show
        Shawn Heisey added a comment - We should simply comment those to protect them if there is no agreement. Personally, I want the logs to point me to the correct class. I'm with Mark on this. Someone who has no interest in the code won't know what it means, but if they send the logs to someone who does have that interest (including the mailing list or a Jira issue), then logging a different class name will slow down efforts to help. I can't think of a situation where a shared logger is actually a good idea, but that might be a failure of imagination on my part. If there IS a good reason to do it, that reason should definitely be in a comment on the logger creation.
        Hide
        Mark Miller added a comment -

        I think for the tlog example, the counter argument is things like controlling all the tlog logging with one class even, even if it spans some helper classes.

        That's fine, but given the current state of things, lots of benefits in simplifying.

        Show
        Mark Miller added a comment - I think for the tlog example, the counter argument is things like controlling all the tlog logging with one class even, even if it spans some helper classes. That's fine, but given the current state of things, lots of benefits in simplifying.
        Hide
        Yonik Seeley added a comment -

        I was coming from the POV of supporting a user/customer... i.e. "turn on DEBUG for these loggers and give me back the output to look at."
        It just didn't seem nice to have to change which loggers needed configuring just because some minor refactors had been done.

        Anyways, I can avoid touching the particular outer class-logger-used in-inner-class scenarios exemplified by TransactionLog

        Actually, it just occurred to me that we don't have much choice in this case, as normal inner classes can't have static members.

        And even for static inner classes that could have their own logger, it doesn't seem great because the settings are apparently not inherited?
        http://it-demystified.blogspot.com/2014/01/log4j-logger-hierarchies-and-java.html

        Show
        Yonik Seeley added a comment - I was coming from the POV of supporting a user/customer... i.e. "turn on DEBUG for these loggers and give me back the output to look at." It just didn't seem nice to have to change which loggers needed configuring just because some minor refactors had been done. Anyways, I can avoid touching the particular outer class-logger-used in-inner-class scenarios exemplified by TransactionLog Actually, it just occurred to me that we don't have much choice in this case, as normal inner classes can't have static members. And even for static inner classes that could have their own logger, it doesn't seem great because the settings are apparently not inherited? http://it-demystified.blogspot.com/2014/01/log4j-logger-hierarchies-and-java.html
        Hide
        Mike Drob added a comment -

        As somebody that looks at customer logs for the majority of my interaction with Solr, I would want Loggers to correspond to file names every single time. So I would actually prefer that inner classes use the outer class loggers, since that makes the code in questions easiest to find. I know this conflicts minorly with the initial stated goal, but I think it's a reasonable exception to make.

        Show
        Mike Drob added a comment - As somebody that looks at customer logs for the majority of my interaction with Solr, I would want Loggers to correspond to file names every single time. So I would actually prefer that inner classes use the outer class loggers, since that makes the code in questions easiest to find. I know this conflicts minorly with the initial stated goal, but I think it's a reasonable exception to make.
        Hide
        Mark Miller added a comment -

        Inner classes are a whole different ballgame. I mean one logger per class file.

        Show
        Mark Miller added a comment - Inner classes are a whole different ballgame. I mean one logger per class file.
        Hide
        Mark Miller added a comment -

        since that makes the code in questions easiest to find

        Still, either I don't understand how inner classes log or that makes no sense to me.

        Show
        Mark Miller added a comment - since that makes the code in questions easiest to find Still, either I don't understand how inner classes log or that makes no sense to me.
        Hide
        Mike Drob added a comment - - edited

        I was imaging a hypothetical

        class A {
            static Logger logA = LoggerFactory.getLogger(A.class);
            
            static class B {
                static Logger logB = LoggerFactory.getLogger(B.class);
            }
        }
        

        In that scenario, I would be in favor of removing logB, since there is no corresponding B.java to look for and I'd have to deduce that it is in A.java. Sometimes this is simple to figure out, sometimes not, most of the time an IDE could find it but bash and vim need some more magic. If this is an artifact of how I do development, then I'm not willing to make a big deal out of it, but I figured it's worth mentioning it once.

        Show
        Mike Drob added a comment - - edited I was imaging a hypothetical class A { static Logger logA = LoggerFactory.getLogger(A.class); static class B { static Logger logB = LoggerFactory.getLogger(B.class); } } In that scenario, I would be in favor of removing logB , since there is no corresponding B.java to look for and I'd have to deduce that it is in A.java . Sometimes this is simple to figure out, sometimes not, most of the time an IDE could find it but bash and vim need some more magic. If this is an artifact of how I do development, then I'm not willing to make a big deal out of it, but I figured it's worth mentioning it once.
        Hide
        Anshum Gupta added a comment -

        From how I understand this discussion, I think Mike and Mark are exactly on the same page and I agree with it. Here's what I think: All 'files' should use a single logger i.e. Inner classes reuse the logger but other than that, every class should have it's own logger. It'd make pointing at the file w.r.t the log entry easier and more obvious.

        In any case, I think let's move forward with the least invasive approach and get this resolved. We can always re-iterate in a manner that doesn't undo any of the work done here so users don't have a hard time understanding and adapting to the changes.

        Show
        Anshum Gupta added a comment - From how I understand this discussion, I think Mike and Mark are exactly on the same page and I agree with it. Here's what I think: All 'files' should use a single logger i.e. Inner classes reuse the logger but other than that, every class should have it's own logger. It'd make pointing at the file w.r.t the log entry easier and more obvious. In any case, I think let's move forward with the least invasive approach and get this resolved. We can always re-iterate in a manner that doesn't undo any of the work done here so users don't have a hard time understanding and adapting to the changes.
        Hide
        Mark Miller added a comment -

        In that scenario, I would be in favor of removing logB, since there is no corresponding B.java to look for and I'd have to deduce that it is in A.java.

        Output of:

        package.A$B blah blah log message

        means go to package, class A, inner class B.

        Show
        Mark Miller added a comment - In that scenario, I would be in favor of removing logB, since there is no corresponding B.java to look for and I'd have to deduce that it is in A.java. Output of: package.A$B blah blah log message means go to package, class A, inner class B.
        Hide
        Mark Miller added a comment -

        Anyway, beside multiple loggers per class file being annoying, controlling logging for them is kind of niche, special case ugliness, and I think even how the output for them is done depends on the logging impl and the formatter impl for that logging impl.

        Def not arguing for inner class loggers.

        Show
        Mark Miller added a comment - Anyway, beside multiple loggers per class file being annoying, controlling logging for them is kind of niche, special case ugliness, and I think even how the output for them is done depends on the logging impl and the formatter impl for that logging impl. Def not arguing for inner class loggers.
        Hide
        Jason Gerlowski added a comment -

        Ok, checking back in.

        Attached is my first attempt at this patch. It applies to trunk as of the time of this comment (with a few minutes delta). It has the following changes:

        1.) All loggers now use copy-paste safe "MethodHandles" style declaration.

        2.) I deleted loggers in classes where I found they weren't being used at all. (Since I was switching many of them from public to private, I found I didn't want to add warnings about unused-fields, so I deleted them)

        3.) No loggers are shared outside of the Java file in which they were declared. However, some outer class loggers are still used by inner classes. It seemed like this was the consensus we came to on this.

        I'm happy to make any changes to (1), (2), or (3) above. Hopefully I didn't misread the consensus here.

        One last question- I was unsure of whether this change merited a note in CHANGES.txt. It is mostly a reorganization, but it will change a few log messages. This patch doesn't add a note to CHANGES.txt, but I'm happy to add one if it merits one. I wasn't sure whether all changes make it into that file, or just ones that are of particular interest to end users.

        Show
        Jason Gerlowski added a comment - Ok, checking back in. Attached is my first attempt at this patch. It applies to trunk as of the time of this comment (with a few minutes delta). It has the following changes: 1.) All loggers now use copy-paste safe "MethodHandles" style declaration. 2.) I deleted loggers in classes where I found they weren't being used at all. (Since I was switching many of them from public to private, I found I didn't want to add warnings about unused-fields, so I deleted them) 3.) No loggers are shared outside of the Java file in which they were declared. However, some outer class loggers are still used by inner classes. It seemed like this was the consensus we came to on this. I'm happy to make any changes to (1), (2), or (3) above. Hopefully I didn't misread the consensus here. One last question- I was unsure of whether this change merited a note in CHANGES.txt. It is mostly a reorganization, but it will change a few log messages. This patch doesn't add a note to CHANGES.txt, but I'm happy to add one if it merits one. I wasn't sure whether all changes make it into that file, or just ones that are of particular interest to end users.
        Hide
        Shawn Heisey added a comment -

        I think it definitely is worthy of mention in CHANGES.txt, if only for the sheer size of the patch. It's probably worth mentioning in the "upgrading from" section as well as the issue list, with a paragraph similar to this:

        * Logger declarations in most source files have changed to code that
          no longer needs to explicitly state the class name.  This fixes situations
          where a logger for a different class was incorrectly used. See SOLR-8324
          and its sub-issues for details.
        

        On SOLR-8324, Uwe mentioned a way of doing the declaration without imports – fully qualified class names in the code. I wonder if that's the way we should do this. I have no strong opinion on the matter.

        Show
        Shawn Heisey added a comment - I think it definitely is worthy of mention in CHANGES.txt, if only for the sheer size of the patch. It's probably worth mentioning in the "upgrading from" section as well as the issue list, with a paragraph similar to this: * Logger declarations in most source files have changed to code that no longer needs to explicitly state the class name. This fixes situations where a logger for a different class was incorrectly used. See SOLR-8324 and its sub-issues for details. On SOLR-8324 , Uwe mentioned a way of doing the declaration without imports – fully qualified class names in the code. I wonder if that's the way we should do this. I have no strong opinion on the matter.
        Hide
        Jason Gerlowski added a comment -

        OK, ill add a note in CHANGES.Txt when I get back to a computer.

        I don't see a ton of value in getting rid of Logger imports, but I'm happy to add that if people want to see it added.

        Show
        Jason Gerlowski added a comment - OK, ill add a note in CHANGES.Txt when I get back to a computer. I don't see a ton of value in getting rid of Logger imports, but I'm happy to add that if people want to see it added.
        Hide
        Jason Gerlowski added a comment -

        This patch includes a note in CHANGES.txt. Shawn, I used your paragraph verbatim for the "upgrading from" section. Well worded; thanks!

        Show
        Jason Gerlowski added a comment - This patch includes a note in CHANGES.txt. Shawn, I used your paragraph verbatim for the "upgrading from" section. Well worded; thanks!
        Hide
        Anshum Gupta added a comment -

        Wow that's a good 400kb patch! I'd like to change the CHANGES.txt entry to say "Standardize and fix logger creation and usage so that they are't shared across java class files"
        or something. It's certainly would be good to call it out explicitly that we want to move forward with this as more or less the standard, so that no one needs to revisit this and fix it again.

        +1 from my end. If others are fine, I can commit this today/tomorrow so that it makes it to 5.4.

        Show
        Anshum Gupta added a comment - Wow that's a good 400kb patch! I'd like to change the CHANGES.txt entry to say "Standardize and fix logger creation and usage so that they are't shared across java class files" or something. It's certainly would be good to call it out explicitly that we want to move forward with this as more or less the standard, so that no one needs to revisit this and fix it again. +1 from my end. If others are fine, I can commit this today/tomorrow so that it makes it to 5.4.
        Hide
        Jason Gerlowski added a comment -

        I've updated the BUG-FIXES list item in CHANGES.txt to line up more with Anshum's suggested text.

        Show
        Jason Gerlowski added a comment - I've updated the BUG-FIXES list item in CHANGES.txt to line up more with Anshum's suggested text.
        Hide
        Shawn Heisey added a comment -

        The placement will be up to Anshum as he's said he'll commit it, but I would put this under either "Other Changes" or "Optimizations" rather than "Bug Fixes". I view it as more of an improvement than a fix.

        Show
        Shawn Heisey added a comment - The placement will be up to Anshum as he's said he'll commit it, but I would put this under either "Other Changes" or "Optimizations" rather than "Bug Fixes". I view it as more of an improvement than a fix.
        Hide
        Jason Gerlowski added a comment -

        OK, still getting used to the conventions here. Whatever you/he think makes sense is fine with me.

        Show
        Jason Gerlowski added a comment - OK, still getting used to the conventions here. Whatever you/he think makes sense is fine with me.
        Hide
        Anshum Gupta added a comment -

        Yes, I'll just run the tests now and commit. and yes, this is more of something that belongs to the "other changes" category.

        Show
        Anshum Gupta added a comment - Yes, I'll just run the tests now and commit. and yes, this is more of something that belongs to the "other changes" category.
        Hide
        Anshum Gupta added a comment -

        Hmm the patch applied cleanly but there were compilation errors which I discovered when I tried to run the tests. Most of those were because:

        • Extended classes e.g. HdfsUpdateLog, CdcrTransactionLog was trying to reuse the logger from the parent class, which no longer is possible as the loggers are all private. I've added new loggers for that purpose.
        • SolrCore.log was being used at multiple places e.g. in RequestHandlerBase. Unless there's something I'm completely missing here, it doesn't make sense to do that in line what Mike Drob mentioned about debugging through logs. I've changed that to use the file specific logger instead (even for abstract classes).
        • PreAnalyzedUpdateProcessorFactory is where it's more interesting. There are 2 classes in there public PreAnalyzedUpdateProcessorFactory, and PreAnalyzedUpdateProcessor. The logger is required in PreAnalyzedUpdateProcessor but the file is named PreAnalyzedUpdateProcessorFactory which would mean that this issue still remains in this case here. We should just move this to another file, but I'll use another JIRA for that and not mix the 2 up.

        I'm almost done with fixing the compilation issues but will take some more time tonight before committing this. If there are any red flags that someone notices here, please raise your hand.

        Show
        Anshum Gupta added a comment - Hmm the patch applied cleanly but there were compilation errors which I discovered when I tried to run the tests. Most of those were because: Extended classes e.g. HdfsUpdateLog, CdcrTransactionLog was trying to reuse the logger from the parent class, which no longer is possible as the loggers are all private. I've added new loggers for that purpose. SolrCore.log was being used at multiple places e.g. in RequestHandlerBase. Unless there's something I'm completely missing here, it doesn't make sense to do that in line what Mike Drob mentioned about debugging through logs. I've changed that to use the file specific logger instead (even for abstract classes). PreAnalyzedUpdateProcessorFactory is where it's more interesting. There are 2 classes in there public PreAnalyzedUpdateProcessorFactory, and PreAnalyzedUpdateProcessor. The logger is required in PreAnalyzedUpdateProcessor but the file is named PreAnalyzedUpdateProcessorFactory which would mean that this issue still remains in this case here. We should just move this to another file, but I'll use another JIRA for that and not mix the 2 up. I'm almost done with fixing the compilation issues but will take some more time tonight before committing this. If there are any red flags that someone notices here, please raise your hand.
        Hide
        Anshum Gupta added a comment -

        Patch with fixes mentioned in my last comment and updated to trunk.

        Show
        Anshum Gupta added a comment - Patch with fixes mentioned in my last comment and updated to trunk.
        Hide
        Mike Drob added a comment -

        Yea, I was concerned that there would be some difficulty extracting loggers used in multiple places. I initially envisioned doing all the easy stuff in one JIRA and then taking care of the difficult stuff in separate child tasks as necessary.

        Show
        Mike Drob added a comment - Yea, I was concerned that there would be some difficulty extracting loggers used in multiple places. I initially envisioned doing all the easy stuff in one JIRA and then taking care of the difficult stuff in separate child tasks as necessary.
        Hide
        Erick Erickson added a comment -

        CdcrTransactionLog is not in the 5x code line, so that'll be one "interesting" thing to reconcile.....

        FYI

        Show
        Erick Erickson added a comment - CdcrTransactionLog is not in the 5x code line, so that'll be one "interesting" thing to reconcile..... FYI
        Hide
        Anshum Gupta added a comment -

        If you mean the svn merge, hopefully it'll take care of it.

        Show
        Anshum Gupta added a comment - If you mean the svn merge, hopefully it'll take care of it.
        Hide
        Anshum Gupta added a comment -

        I couldn't think of an easy way to split it out other than not doing this for the tests or certain parts of the code. I wanted to keep it easy but it's been a rabbit hole.
        I think it's a ton of volume but not really intrusive in terms of functional impact. What do you think?

        Show
        Anshum Gupta added a comment - I couldn't think of an easy way to split it out other than not doing this for the tests or certain parts of the code. I wanted to keep it easy but it's been a rabbit hole. I think it's a ton of volume but not really intrusive in terms of functional impact. What do you think?
        Hide
        Anshum Gupta added a comment -

        OpenCloseCoreStressTest is another file with multiple classes. I've declared multiple loggers in there but I think we should move away from the practice.
        Putting up a patch in a bit with the tests fixed as well.

        Show
        Anshum Gupta added a comment - OpenCloseCoreStressTest is another file with multiple classes. I've declared multiple loggers in there but I think we should move away from the practice. Putting up a patch in a bit with the tests fixed as well.
        Hide
        Anshum Gupta added a comment -

        Updated patch, Running tests on this one.

        Show
        Anshum Gupta added a comment - Updated patch, Running tests on this one.
        Hide
        Uwe Schindler added a comment -

        +1 LGTM

        Show
        Uwe Schindler added a comment - +1 LGTM
        Hide
        Jason Gerlowski added a comment -

        Looks good to me; thanks Anshum.

        Show
        Jason Gerlowski added a comment - Looks good to me; thanks Anshum.
        Hide
        Uwe Schindler added a comment -

        Hi,
        I had an idea a minute ago how to prevent incorrectly declared loggers. In root's build.xml we have the source-patterns ant task (that greps on source files for "violations"). We could add a regular expression for finding "bad loggers" and fail "ant validate" because of that.

        We can commit this afterwards, I can work on some regexes to detect this (some combinations of positive/negative patterns).

        Show
        Uwe Schindler added a comment - Hi, I had an idea a minute ago how to prevent incorrectly declared loggers. In root's build.xml we have the source-patterns ant task (that greps on source files for "violations"). We could add a regular expression for finding "bad loggers" and fail "ant validate" because of that. We can commit this afterwards, I can work on some regexes to detect this (some combinations of positive/negative patterns).
        Hide
        Uwe Schindler added a comment - - edited

        Hi,
        I wrote a pattern detector, basically it does the following: If the java file contains "org.slf4j.LoggerFactory", it will look for some pattern matching 3 occurences of either "private", "static", "final", followed by "LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());"

        Without this patch, it reports 433 errors, when the patch posted here is applied it still detects some violations:

        [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/core/SolrEventListener.java
        [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/search/SolrCache.java
        [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/search/SurroundQParserPlugin.java
        [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/servlet/CheckLoggingConfiguration.java
        [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/update/processor/IgnoreCommitOptimizeUpdateProcessorFactory.java
        [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/update/processor/LogUpdateProcessorFactory.java
        [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/test/org/apache/solr/logging/TestLogWatcher.java
        

        Maybe these are false positives, but the pattern looks quite good, although it might not be able to correctly handle crazy Eclipse newline formatting.

        Show
        Uwe Schindler added a comment - - edited Hi, I wrote a pattern detector, basically it does the following: If the java file contains "org.slf4j.LoggerFactory", it will look for some pattern matching 3 occurences of either "private", "static", "final", followed by "LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());" Without this patch, it reports 433 errors, when the patch posted here is applied it still detects some violations: [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/core/SolrEventListener.java [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/search/SolrCache.java [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/search/SurroundQParserPlugin.java [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/servlet/CheckLoggingConfiguration.java [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/update/processor/IgnoreCommitOptimizeUpdateProcessorFactory.java [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/java/org/apache/solr/update/processor/LogUpdateProcessorFactory.java [source-patterns] invalid logging pattern [not private static final, uses static class name]: solr/core/src/test/org/apache/solr/logging/TestLogWatcher.java Maybe these are false positives, but the pattern looks quite good, although it might not be able to correctly handle crazy Eclipse newline formatting.
        Hide
        Uwe Schindler added a comment -

        Small improvements in pattern, only scan *.java files.

        The errors reported are now mainly because the logger is not private.

        Show
        Uwe Schindler added a comment - Small improvements in pattern, only scan *.java files. The errors reported are now mainly because the logger is not private.
        Hide
        Erick Erickson added a comment -

        Just alerting you to this if you run into it.

        Show
        Erick Erickson added a comment - Just alerting you to this if you run into it.
        Hide
        Jason Gerlowski added a comment - - edited

        I think having a source-patterns check is a good idea in general, but it might not be as useful as we hope.

        For example, a number of the violations in Uwe's comment above are (arguably) false positives.

        1.) Several interfaces contain Logger declarations. These declarations can't be private since interfaces can't contain private members. These Loggers should probably be removed entirely. I didn't do this in my initial patch because I was making an effort to be conservative about what I changed, but I really don't think there's a reason to keep them around. This affects SolrEventListener, and SolrCache.

        2.) Some Java files contain two non-nested classes which share loggers. For example:

             //All code in file A.java
        
             class A {
                  static final Logger log = ....;
             }
        
             class B {
                  static final Logger log = B.log;
             }
        

        Because the classes aren't nested, the logger can't be a private member. The classes can be made to use separate loggers, but this would break Mike Drob's point that:

        As somebody that looks at customer logs for the majority of my interaction with Solr, I would want Loggers to correspond to file names every single time.

        This corner case affects SurroundQParserPlugin, IgnoreCommitOptimizeUpdateProcessorFactory, and LogUpdateProcessorFactory.

        3.) Some of the Java files reported above are actually testing log4j configuration/setup. Often, these tests declare Loggers in non-standard ways. For example TestLogWatcher declares a Logger as a local variable in a test method.

        To clarify my point a bit, I'm not saying that a we shouldn't have a source-analysis check for this. But the fact that there are already corner cases/edge cases probably means this shouldn't be used as pass/fail criteria for ant validate. And unfortunately it might mean that the check will be ignored more often than not (since it doesn't really have any "teeth").

        Show
        Jason Gerlowski added a comment - - edited I think having a source-patterns check is a good idea in general, but it might not be as useful as we hope. For example, a number of the violations in Uwe's comment above are (arguably) false positives. 1.) Several interfaces contain Logger declarations. These declarations can't be private since interfaces can't contain private members. These Loggers should probably be removed entirely. I didn't do this in my initial patch because I was making an effort to be conservative about what I changed, but I really don't think there's a reason to keep them around. This affects SolrEventListener , and SolrCache . 2.) Some Java files contain two non-nested classes which share loggers. For example: //All code in file A.java class A { static final Logger log = ....; } class B { static final Logger log = B.log; } Because the classes aren't nested, the logger can't be a private member. The classes can be made to use separate loggers, but this would break Mike Drob's point that: As somebody that looks at customer logs for the majority of my interaction with Solr, I would want Loggers to correspond to file names every single time. This corner case affects SurroundQParserPlugin , IgnoreCommitOptimizeUpdateProcessorFactory , and LogUpdateProcessorFactory . 3.) Some of the Java files reported above are actually testing log4j configuration/setup. Often, these tests declare Loggers in non-standard ways. For example TestLogWatcher declares a Logger as a local variable in a test method. To clarify my point a bit, I'm not saying that a we shouldn't have a source-analysis check for this. But the fact that there are already corner cases/edge cases probably means this shouldn't be used as pass/fail criteria for ant validate . And unfortunately it might mean that the check will be ignored more often than not (since it doesn't really have any "teeth").
        Hide
        Anshum Gupta added a comment -

        Thanks Uwe! The only thing that's stopping me is, I have a trans atlantic flight, and more which might keep me offline for about 40 hours later today.
        I don't want to commit this and be gone so
        1. I can either commit it when I'm back online
        2. Someone else can commit this and take it from here

        P.S: The RequestLoggingTest is still failing and I'm not sure if that test makes sense any more or not. Gregory Chanan, seem like you'd have a better idea about that one. Thoughts?

        Show
        Anshum Gupta added a comment - Thanks Uwe! The only thing that's stopping me is, I have a trans atlantic flight, and more which might keep me offline for about 40 hours later today. I don't want to commit this and be gone so 1. I can either commit it when I'm back online 2. Someone else can commit this and take it from here P.S: The RequestLoggingTest is still failing and I'm not sure if that test makes sense any more or not. Gregory Chanan , seem like you'd have a better idea about that one. Thoughts?
        Hide
        Anshum Gupta added a comment -

        Jason Gerlowski About loggers in interfaces, with Java8, you could write a default implementation for a method as part of the interface declaration, thereby requiring the logger. I know this was brought up a while ago when we decided to move to Java 8 on trunk.

        Show
        Anshum Gupta added a comment - Jason Gerlowski About loggers in interfaces, with Java8, you could write a default implementation for a method as part of the interface declaration, thereby requiring the logger. I know this was brought up a while ago when we decided to move to Java 8 on trunk.
        Hide
        Jason Gerlowski added a comment -

        Makes sense, glad I didn't delete them then. That makes it a stronger case for not failing ant validate for these violations then.

        Show
        Jason Gerlowski added a comment - Makes sense, glad I didn't delete them then. That makes it a stronger case for not failing ant validate for these violations then.
        Hide
        Uwe Schindler added a comment - - edited

        Hi for the other changes: All fixes are quite easy:
        1) maybe we should remove loggers from interfaces, without any static methods (in Java 7 there cannot be any), it makes no sense to have them. So put them into the impl class. In Lucene trunk we can use loggers, but as long as we parallel do maintain the 5.x branch we should as close as possible between source code.
        2) The non-nested classes are an anti-pattern from earlier Java 1.0 days. We should simply remove them (move the parallel class as static inner class into the main class). I did this several times whenever I saw them. Those constructs generally have the problem that it does not work with incremental compile (javac fails to compile incrementally when you change source files, because it cannot corelate the class files to source files). So we should fix them.
        3) The tests can get a fake logger or let's exclude them. There is also the CheckLoggingConfiguration in servlet package. This should be excluded from checks (which is quite easy to do).

        If this is fixed we are fine.

        Show
        Uwe Schindler added a comment - - edited Hi for the other changes: All fixes are quite easy: 1) maybe we should remove loggers from interfaces, without any static methods (in Java 7 there cannot be any), it makes no sense to have them. So put them into the impl class. In Lucene trunk we can use loggers, but as long as we parallel do maintain the 5.x branch we should as close as possible between source code. 2) The non-nested classes are an anti-pattern from earlier Java 1.0 days. We should simply remove them (move the parallel class as static inner class into the main class). I did this several times whenever I saw them. Those constructs generally have the problem that it does not work with incremental compile (javac fails to compile incrementally when you change source files, because it cannot corelate the class files to source files). So we should fix them. 3) The tests can get a fake logger or let's exclude them. There is also the CheckLoggingConfiguration in servlet package. This should be excluded from checks (which is quite easy to do). If this is fixed we are fine.
        Hide
        Mark Miller added a comment -

        +1 to Uwe's validator and making the changes for it to pass. Awesome.

        Show
        Mark Miller added a comment - +1 to Uwe's validator and making the changes for it to pass. Awesome.
        Hide
        Anshum Gupta added a comment -

        +1 to Uwe's validator and making those changes but here's my concern with 1. Once we move to Java 8; possibly in the next couple of releases. Once that happens, we might have developers wanting to write default implementation in interfaces and we'd have to fix this then anyways so I'm not sure if we want to remove that for now.

        Show
        Anshum Gupta added a comment - +1 to Uwe's validator and making those changes but here's my concern with 1. Once we move to Java 8; possibly in the next couple of releases. Once that happens, we might have developers wanting to write default implementation in interfaces and we'd have to fix this then anyways so I'm not sure if we want to remove that for now.
        Hide
        Jason Gerlowski added a comment -

        Upon slightly closer inspection, the two interfaces that contain loggers (SolrCache and SolrEventListener) aren't actually used anywhere. So they could/should be deleted.

        With that in mind, do we even want to allow interfaces to have loggers for now? There's a solid rationale when we move to Java8, sure. But I'd argue that until then, interface-loggers shouldn't be encouraged/allowed. Changing the source-check to allow loggers in interfaces could be part of the move-to-Java8 effort (as long as we can make sure this doesn't get lost when the move actually happens...that's my main qualm).

        Show
        Jason Gerlowski added a comment - Upon slightly closer inspection, the two interfaces that contain loggers ( SolrCache and SolrEventListener ) aren't actually used anywhere. So they could/should be deleted. With that in mind, do we even want to allow interfaces to have loggers for now? There's a solid rationale when we move to Java8, sure. But I'd argue that until then, interface-loggers shouldn't be encouraged/allowed. Changing the source-check to allow loggers in interfaces could be part of the move-to-Java8 effort (as long as we can make sure this doesn't get lost when the move actually happens...that's my main qualm).
        Hide
        Mark Miller added a comment -

        Indeed. We dont need to burn bridges that are not built yet. When we move to Java 8 and someone is dying to add a logger to an interface they can open an issue.

        Show
        Mark Miller added a comment - Indeed. We dont need to burn bridges that are not built yet. When we move to Java 8 and someone is dying to add a logger to an interface they can open an issue.
        Hide
        Jason Gerlowski added a comment -

        Cool, If Anshum's fine w/ that plan, I'm happy to update the patch with that in mind (though the changes are pretty trivial). But I'll refrain from making any changes until he's able to chime in.

        Show
        Jason Gerlowski added a comment - Cool, If Anshum's fine w/ that plan, I'm happy to update the patch with that in mind (though the changes are pretty trivial). But I'll refrain from making any changes until he's able to chime in.
        Hide
        Anshum Gupta added a comment -

        Sure, sounds good to me. It's reasonable and also helps us move forward at this point.

        I'm not going to be able to look at the patch for about 16-18 more hours from now but should be back online after that. Jet lagged but online.
        So, I'll take a look at it then.

        Show
        Anshum Gupta added a comment - Sure, sounds good to me. It's reasonable and also helps us move forward at this point. I'm not going to be able to look at the patch for about 16-18 more hours from now but should be back online after that. Jet lagged but online. So, I'll take a look at it then.
        Hide
        Jason Gerlowski added a comment -

        I've made the changes that Uwe suggested a few comments ago.

        1.) Interface loggers have been removed.

        2.) non-nested classes that required loggers to be non-private were changed to internal static classes.

        3.) Test classes that used non-private loggers have been massaged to comply with the source-pattern check (or in one case, excluded).

        As Anshum pointed out in an earlier comment, RequestLoggingTest still seems to fail with this patch. It's unclear to me how relevant this test still is.

        Show
        Jason Gerlowski added a comment - I've made the changes that Uwe suggested a few comments ago. 1.) Interface loggers have been removed. 2.) non-nested classes that required loggers to be non-private were changed to internal static classes. 3.) Test classes that used non-private loggers have been massaged to comply with the source-pattern check (or in one case, excluded). As Anshum pointed out in an earlier comment, RequestLoggingTest still seems to fail with this patch. It's unclear to me how relevant this test still is.
        Hide
        Gregory Chanan added a comment -

        RequestLoggingTest is still relevant – it checks the output of specific loggers. I don't quite understand why it doesn't work though – does the MethodHandles change the name of the logger?

        Show
        Gregory Chanan added a comment - RequestLoggingTest is still relevant – it checks the output of specific loggers. I don't quite understand why it doesn't work though – does the MethodHandles change the name of the logger?
        Hide
        Gregory Chanan added a comment -

        BTW changing the requestLog in SolrCore.java to:

         requestLog = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass().getName()+ ".Request");

        gets the test to pass for me.

        Show
        Gregory Chanan added a comment - BTW changing the requestLog in SolrCore.java to: requestLog = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass().getName()+ ".Request" ); gets the test to pass for me.
        Hide
        Anshum Gupta added a comment -

        Thanks Greg and Jason.

        I'll take a look at this patch and commit this after the tests pass, along with Uwe's awesome validator

        Show
        Anshum Gupta added a comment - Thanks Greg and Jason. I'll take a look at this patch and commit this after the tests pass, along with Uwe's awesome validator
        Hide
        Uwe Schindler added a comment - - edited

        This is the correct fix. Class#toString() (which is used implicit by the string concat here), returns "class oas.SolrCore" including "class " prefix. This is a common programming error.

        By the way, this declaration passes the checker, because there is already another valid logger declaration there. This may also be a workarund for other places: Just declare a correct logger in the same file and you are able to do other, incorrect logger declarations

        Show
        Uwe Schindler added a comment - - edited This is the correct fix. Class#toString() (which is used implicit by the string concat here), returns "class oas.SolrCore" including "class " prefix. This is a common programming error. By the way, this declaration passes the checker, because there is already another valid logger declaration there. This may also be a workarund for other places: Just declare a correct logger in the same file and you are able to do other, incorrect logger declarations
        Hide
        Uwe Schindler added a comment -

        I will try the patch later, but looks fine to me; small issues:

        • the changes entry is missing a space before the names
        • the excluded file in the ANT pattern in the checker should be maybe a bit more specific. The only backside with the exclusion here is that it omits all checks on this file, but that's acceptable. The CheckLoggingConfiguration class is not really containing much code...
        Show
        Uwe Schindler added a comment - I will try the patch later, but looks fine to me; small issues: the changes entry is missing a space before the names the excluded file in the ANT pattern in the checker should be maybe a bit more specific. The only backside with the exclusion here is that it omits all checks on this file, but that's acceptable. The CheckLoggingConfiguration class is not really containing much code...
        Hide
        Uwe Schindler added a comment -

        The attached patch no longer applies cleanly. You have to "svn update" to revision 1717026. Then it works and compiles, but updating then to trunk again leads to conflicts and compile errors. It looks like this patch has so many changes so it should be applied ASAP.

        Gregory Chanan's fix for SolrCore should be applied, too.

        Show
        Uwe Schindler added a comment - The attached patch no longer applies cleanly. You have to "svn update" to revision 1717026. Then it works and compiles, but updating then to trunk again leads to conflicts and compile errors. It looks like this patch has so many changes so it should be applied ASAP. Gregory Chanan 's fix for SolrCore should be applied, too.
        Hide
        Jason Gerlowski added a comment -

        I've updated the patch to sit on top of the latest trunk changes (as of maybe an hour or two ago).

        I also added the fix that Greg mentioned below. With it, RequestLoggingTest passes now!

        I'm in the process of running the tests in the background, and I'll post again if/when I see them pass.

        Show
        Jason Gerlowski added a comment - I've updated the patch to sit on top of the latest trunk changes (as of maybe an hour or two ago). I also added the fix that Greg mentioned below. With it, RequestLoggingTest passes now! I'm in the process of running the tests in the background, and I'll post again if/when I see them pass.
        Hide
        Jason Gerlowski added a comment -

        Tests passed for me, so I think this patch should be looking good.

        Show
        Jason Gerlowski added a comment - Tests passed for me, so I think this patch should be looking good.
        Hide
        Anshum Gupta added a comment -

        Patch with a few more fixes that I came across while running ant clean, test, validate, and precommit. The tests pass and all looks good. I'll commit this now.

        Might have to deal with branch_5x merge next.

        Show
        Anshum Gupta added a comment - Patch with a few more fixes that I came across while running ant clean, test, validate, and precommit. The tests pass and all looks good. I'll commit this now. Might have to deal with branch_5x merge next.
        Hide
        ASF subversion and git services added a comment -

        Commit 1717590 from Anshum Gupta in branch 'dev/trunk'
        [ https://svn.apache.org/r1717590 ]

        SOLR-8330: Standardize and fix logger creation and usage so that they aren't shared across source files.

        Show
        ASF subversion and git services added a comment - Commit 1717590 from Anshum Gupta in branch 'dev/trunk' [ https://svn.apache.org/r1717590 ] SOLR-8330 : Standardize and fix logger creation and usage so that they aren't shared across source files.
        Hide
        Uwe Schindler added a comment -

        +1 Thanks!

        Show
        Uwe Schindler added a comment - +1 Thanks!
        Hide
        ASF subversion and git services added a comment -

        Commit 1717604 from Anshum Gupta in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1717604 ]

        SOLR-8330: Standardize and fix logger creation and usage so that they aren't shared across source files.(merge from trunk)

        Show
        ASF subversion and git services added a comment - Commit 1717604 from Anshum Gupta in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1717604 ] SOLR-8330 : Standardize and fix logger creation and usage so that they aren't shared across source files.(merge from trunk)
        Hide
        Anshum Gupta added a comment -

        I've committed it to both trunk and 5x. I think we should also commit this to 5.4, any thoughts?

        Thank you Jason, Uwe and everyone else !

        Show
        Anshum Gupta added a comment - I've committed it to both trunk and 5x. I think we should also commit this to 5.4, any thoughts? Thank you Jason, Uwe and everyone else !
        Hide
        Jason Gerlowski added a comment -

        Thank you Anshum!

        I think it'd be nice to squeeze into 5.4. Even though this is a big change from a LOC perspective, I think it's pretty safe, and is a net gain for log-readability. It'd be nice to get out to people. Though, as a partial author, I must admit a little bias :-p

        Show
        Jason Gerlowski added a comment - Thank you Anshum! I think it'd be nice to squeeze into 5.4. Even though this is a big change from a LOC perspective, I think it's pretty safe, and is a net gain for log-readability. It'd be nice to get out to people. Though, as a partial author, I must admit a little bias :-p
        Hide
        ASF subversion and git services added a comment -

        Commit 1717707 from Anshum Gupta in branch 'dev/branches/lucene_solr_5_4'
        [ https://svn.apache.org/r1717707 ]

        SOLR-8330: Standardize and fix logger creation and usage so that they aren't shared across source files.(merge from branch_5x)

        Show
        ASF subversion and git services added a comment - Commit 1717707 from Anshum Gupta in branch 'dev/branches/lucene_solr_5_4' [ https://svn.apache.org/r1717707 ] SOLR-8330 : Standardize and fix logger creation and usage so that they aren't shared across source files.(merge from branch_5x)
        Hide
        Anshum Gupta added a comment -

        It's now in 5.4.

        Show
        Anshum Gupta added a comment - It's now in 5.4.
        Hide
        Ishan Chattopadhyaya added a comment - - edited

        The MethodHandles.lookup() creates an extra object for the Lookup inner class of MethodHandles [0].

        Isn't it memory/gc wise expensive, esp. given that some of the shortlived, often created objects (like AtomicUpdateDocumentMerger, DistributedUpdateProcessor etc.), would be creating their own logger, thus implying many extra MethodHandles.Lookup objects for every request? Is that a significant problem, or am I over estimating the problem?

        [0] - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/java/lang/invoke/MethodHandles.java#MethodHandles.lookup%28%29

        Show
        Ishan Chattopadhyaya added a comment - - edited The MethodHandles.lookup() creates an extra object for the Lookup inner class of MethodHandles [0] . Isn't it memory/gc wise expensive, esp. given that some of the shortlived, often created objects (like AtomicUpdateDocumentMerger, DistributedUpdateProcessor etc.), would be creating their own logger, thus implying many extra MethodHandles.Lookup objects for every request? Is that a significant problem, or am I over estimating the problem? [0] - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7-b147/java/lang/invoke/MethodHandles.java#MethodHandles.lookup%28%29
        Hide
        Uwe Schindler added a comment - - edited

        The loggers are static, so created once. There is no per-request cost!

        In addition, the Lookup objects are never created on heap, because they never escape the declaration, so they are optimized away by hotspot, see escape analysis.

        Show
        Uwe Schindler added a comment - - edited The loggers are static, so created once. There is no per-request cost! In addition, the Lookup objects are never created on heap, because they never escape the declaration, so they are optimized away by hotspot, see escape analysis .
        Hide
        Ishan Chattopadhyaya added a comment -

        Ah, I was missing the static part; indeed no per-request cost. Good to learn about the escape analysis; thanks!

        Show
        Ishan Chattopadhyaya added a comment - Ah, I was missing the static part; indeed no per-request cost. Good to learn about the escape analysis; thanks!
        Hide
        Uwe Schindler added a comment -

        Good to learn about the escape analysis; thanks!

        This is one of the most important Hotspot optimizations, especially for Java 8! Lambdas and the fluent stream API based on collections would be slow without escape analysis. Fluent APIs like int evenSquareSum = Arrays.stream(intArray).filter(i -> i % 2 == 0).map(i -> i * i).sum(); make profit from that because those APIs return new instances of Stream subclasses all the time. Same applies for java.time API. Those never escape the method.

        Show
        Uwe Schindler added a comment - Good to learn about the escape analysis; thanks! This is one of the most important Hotspot optimizations, especially for Java 8! Lambdas and the fluent stream API based on collections would be slow without escape analysis. Fluent APIs like int evenSquareSum = Arrays.stream(intArray).filter(i -> i % 2 == 0).map(i -> i * i).sum(); make profit from that because those APIs return new instances of Stream subclasses all the time. Same applies for java.time API. Those never escape the method.
        Hide
        Mike Drob added a comment -

        This issue looks done - can we resolve it if there is no further work to do?

        Show
        Mike Drob added a comment - This issue looks done - can we resolve it if there is no further work to do?
        Hide
        ASF subversion and git services added a comment -

        Commit 1724826 from Yonik Seeley in branch 'dev/trunk'
        [ https://svn.apache.org/r1724826 ]

        SOLR-8330: fix log4j.properties to match new name of log update processor logger

        Show
        ASF subversion and git services added a comment - Commit 1724826 from Yonik Seeley in branch 'dev/trunk' [ https://svn.apache.org/r1724826 ] SOLR-8330 : fix log4j.properties to match new name of log update processor logger
        Hide
        ASF subversion and git services added a comment -

        Commit 1724827 from Yonik Seeley in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1724827 ]

        SOLR-8330: fix log4j.properties to match new name of log update processor logger

        Show
        ASF subversion and git services added a comment - Commit 1724827 from Yonik Seeley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1724827 ] SOLR-8330 : fix log4j.properties to match new name of log update processor logger

          People

          • Assignee:
            Anshum Gupta
            Reporter:
            Jason Gerlowski
          • Votes:
            0 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development