[jira] [Comment Edited] (SOLR-8330) Restrict logger visibility throughout the codebase to private so that only the file that declares it can use it
[ https://issues.apache.org/jira/browse/SOLR-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15037614#comment-15037614 ] Uwe Schindler edited comment on SOLR-8330 at 12/3/15 10:29 AM: --- 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|http://docs.oracle.com/javase/7/docs/technotes/guides/vm/performance-enhancements-7.html#escapeAnalysis]. was (Author: thetaphi): The loggers are static, so created once. > Restrict logger visibility throughout the codebase to private so that only > the file that declares it can use it > --- > > Key: SOLR-8330 > URL: https://issues.apache.org/jira/browse/SOLR-8330 > Project: Solr > Issue Type: Sub-task >Affects Versions: Trunk >Reporter: Jason Gerlowski >Assignee: Anshum Gupta > Labels: logging > Fix For: 5.4, Trunk > > Attachments: SOLR-8330-combined.patch, SOLR-8330-detector.patch, > SOLR-8330-detector.patch, SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, > SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch > > > 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. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (SOLR-8330) Restrict logger visibility throughout the codebase to private so that only the file that declares it can use it
[ https://issues.apache.org/jira/browse/SOLR-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15037606#comment-15037606 ] Ishan Chattopadhyaya edited comment on SOLR-8330 at 12/3/15 10:13 AM: -- 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 was (Author: ichattopadhyaya): The MethodHandles.lookup() creates an extra object for the Lookup inner class of MethodHandles. 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? > Restrict logger visibility throughout the codebase to private so that only > the file that declares it can use it > --- > > Key: SOLR-8330 > URL: https://issues.apache.org/jira/browse/SOLR-8330 > Project: Solr > Issue Type: Sub-task >Affects Versions: Trunk >Reporter: Jason Gerlowski >Assignee: Anshum Gupta > Labels: logging > Fix For: 5.4, Trunk > > Attachments: SOLR-8330-combined.patch, SOLR-8330-detector.patch, > SOLR-8330-detector.patch, SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, > SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch > > > 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. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (SOLR-8330) Restrict logger visibility throughout the codebase to private so that only the file that declares it can use it
[ https://issues.apache.org/jira/browse/SOLR-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15033273#comment-15033273 ] Uwe Schindler edited comment on SOLR-8330 at 12/1/15 7:54 AM: -- 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 :-) was (Author: thetaphi): 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. > Restrict logger visibility throughout the codebase to private so that only > the file that declares it can use it > --- > > Key: SOLR-8330 > URL: https://issues.apache.org/jira/browse/SOLR-8330 > Project: Solr > Issue Type: Sub-task >Affects Versions: Trunk >Reporter: Jason Gerlowski >Assignee: Anshum Gupta > Labels: logging > Fix For: Trunk > > Attachments: SOLR-8330-combined.patch, SOLR-8330-detector.patch, > SOLR-8330-detector.patch, SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, > SOLR-8330.patch, SOLR-8330.patch > > > 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. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (SOLR-8330) Restrict logger visibility throughout the codebase to private so that only the file that declares it can use it
[ https://issues.apache.org/jira/browse/SOLR-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15030236#comment-15030236 ] Uwe Schindler edited comment on SOLR-8330 at 11/27/15 9:05 PM: --- 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. was (Author: thetaphi): 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 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. > Restrict logger visibility throughout the codebase to private so that only > the file that declares it can use it > --- > > Key: SOLR-8330 > URL: https://issues.apache.org/jira/browse/SOLR-8330 > Project: Solr > Issue Type: Sub-task >Affects Versions: Trunk >Reporter: Jason Gerlowski >Assignee: Anshum Gupta > Labels: logging > Fix For: Trunk > > Attachments: SOLR-8330-detector.patch, SOLR-8330-detector.patch, > SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, > SOLR-8330.patch > > > 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. -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[jira] [Comment Edited] (SOLR-8330) Restrict logger visibility throughout the codebase to private so that only the file that declares it can use it
[
https://issues.apache.org/jira/browse/SOLR-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15030197#comment-15030197
]
Jason Gerlowski edited comment on SOLR-8330 at 11/27/15 7:46 PM:
-
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:
{code:java}
//All code in file A.java
class A {
static final Logger log = ;
}
class B {
static final Logger log = B.log;
}
{code}
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:
bq. 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").
was (Author: gerlowskija):
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:
{code:java}
//All code in file A.java
class A {
static final Logger log = ;
}
class B {
static final Logger log = B.log;
}
{code}
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:
bq: 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").
> Restrict logger visibility throughout the codebase to private so that only
> the file that declares it can use it
> ---
>
> Key: SOLR-8330
> URL: https://issues.apache.org/jira/browse/SOLR-8330
> Project: Solr
> Issue Type: Sub-task
>Affects Versions: Trunk
>Reporter: Jason Gerlowski
>Assignee: Anshum Gupta
> Labels: logging
> Fix For: Trunk
>
> Attachments: SOLR-8330-detector.patch, SOLR-8330-detector.patch,
> SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.pa
[jira] [Comment Edited] (SOLR-8330) Restrict logger visibility throughout the codebase to private so that only the file that declares it can use it
[
https://issues.apache.org/jira/browse/SOLR-8330?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15030152#comment-15030152
]
Uwe Schindler edited comment on SOLR-8330 at 11/27/15 6:59 PM:
---
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:
{noformat}
[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
{noformat}
Maybe these are false positives, but the pattern looks quite good, although it
might not be able to correctly handle crazy Eclipse newline formatting.
was (Author: thetaphi):
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:
{noformat}
[source-patterns] invalid logging pattern [private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());]:
solr/core/src/java/org/apache/solr/core/SolrEventListener.java
[source-patterns] invalid logging pattern [private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());]:
solr/core/src/java/org/apache/solr/search/SolrCache.java
[source-patterns] invalid logging pattern [private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());]:
solr/core/src/java/org/apache/solr/search/SurroundQParserPlugin.java
[source-patterns] invalid logging pattern [private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());]:
solr/core/src/java/org/apache/solr/servlet/CheckLoggingConfiguration.java
[source-patterns] invalid logging pattern [private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());]:
solr/core/src/java/org/apache/solr/update/processor/IgnoreCommitOptimizeUpdateProcessorFactory.java
[source-patterns] invalid logging pattern [private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());]:
solr/core/src/java/org/apache/solr/update/processor/LogUpdateProcessorFactory.java
[source-patterns] invalid logging pattern [private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());]:
solr/core/src/test/org/apache/solr/logging/TestLogWatcher.java
{noformat}
Maybe these are false positives, but the pattern looks quite good, although it
might not be able to correctly handle crazy Eclipse newline formatting.
> Restrict logger visibility throughout the codebase to private so that only
> the file that declares it can use it
> ---
>
> Key: SOLR-8330
> URL: https://issues.apache.org/jira/browse/SOLR-8330
> Project: Solr
> Issue Type: Sub-task
>Affects Versions: Trunk
>Reporter: Jason Gerlowski
>Assignee: Anshum Gupta
> Labels: logging
> Fix For: Trunk
>
> Attachments: SOLR-8330-detector.patch, SOLR-8330-detector.patch,
> SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch, SOLR-8330.patch,
> SOLR-8330.patch
>
>
> As Mike Drob pointed out in Solr-8324, many loggers
