[jira] [Commented] (HADOOP-12912) Add LOG.isDebugEnabled() guard in Progress.set method
[
https://issues.apache.org/jira/browse/HADOOP-12912?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15383385#comment-15383385
]
Gera Shegalov commented on HADOOP-12912:
Oops I take the above back looking at how isDebugEnabled is implemented
{code}
/**
* Check whether the Log4j Logger used is enabled for DEBUG
priority.
*/
public boolean isDebugEnabled() {
return getLogger().isDebugEnabled();
}
{code}
sorry for the noise!
> Add LOG.isDebugEnabled() guard in Progress.set method
> -
>
> Key: HADOOP-12912
> URL: https://issues.apache.org/jira/browse/HADOOP-12912
> Project: Hadoop Common
> Issue Type: Bug
>Reporter: Tsuyoshi Ozawa
>Assignee: Tsuyoshi Ozawa
> Attachments: HADOOP-12912.001.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-12912) Add LOG.isDebugEnabled() guard in Progress.set method
[ https://issues.apache.org/jira/browse/HADOOP-12912?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15383375#comment-15383375 ] Gera Shegalov commented on HADOOP-12912: isDebugEnabled is worth doing for "best practices" reason imo. I just noticed it in a thread dump. It's much more than a branching instruction. there a bunch of invokevirtual in commons and log4j logging and invokeinterface call behind it. > Add LOG.isDebugEnabled() guard in Progress.set method > - > > Key: HADOOP-12912 > URL: https://issues.apache.org/jira/browse/HADOOP-12912 > Project: Hadoop Common > Issue Type: Bug >Reporter: Tsuyoshi Ozawa >Assignee: Tsuyoshi Ozawa > Attachments: HADOOP-12912.001.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332) - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[jira] [Commented] (HADOOP-12912) Add LOG.isDebugEnabled() guard in Progress.set method
[ https://issues.apache.org/jira/browse/HADOOP-12912?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15199197#comment-15199197 ] Akira AJISAKA commented on HADOOP-12912: bq. 1) the debug() parameters are string literal which are immutable. Agree. I missed that point. Now I'm thinking we don't need to add a guard. Thanks [~liuml07] for the explanation. > Add LOG.isDebugEnabled() guard in Progress.set method > - > > Key: HADOOP-12912 > URL: https://issues.apache.org/jira/browse/HADOOP-12912 > Project: Hadoop Common > Issue Type: Bug >Reporter: Tsuyoshi Ozawa >Assignee: Tsuyoshi Ozawa > Attachments: HADOOP-12912.001.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (HADOOP-12912) Add LOG.isDebugEnabled() guard in Progress.set method
[ https://issues.apache.org/jira/browse/HADOOP-12912?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15189741#comment-15189741 ] Mingliang Liu commented on HADOOP-12912: I'm in favor of replacing log4j logger to slf4j here (as we're doing in other classes). Please refer to [HDFS-8971] I don't quite get the point of performance gain to add a guard here. Adding a guard brings no obvious difference as 1) the debug() parameters are string literal which are immutable. 2) the LOG.debug() should check the log level internally. Would you kindly explain in the description? > Add LOG.isDebugEnabled() guard in Progress.set method > - > > Key: HADOOP-12912 > URL: https://issues.apache.org/jira/browse/HADOOP-12912 > Project: Hadoop Common > Issue Type: Bug >Reporter: Tsuyoshi Ozawa >Assignee: Tsuyoshi Ozawa > Attachments: HADOOP-12912.001.patch > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (HADOOP-12912) Add LOG.isDebugEnabled() guard in Progress.set method
[
https://issues.apache.org/jira/browse/HADOOP-12912?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15189069#comment-15189069
]
Hadoop QA commented on HADOOP-12912:
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m 13s
{color} | {color:blue} Docker mode activated. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s
{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:red}-1{color} | {color:red} test4tests {color} | {color:red} 0m 0s
{color} | {color:red} The patch doesn't appear to include any new or modified
tests. Please justify why no new tests are needed for this patch. Also please
list what manual steps were performed to verify this patch. {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 7m
14s {color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 8m 30s
{color} | {color:green} trunk passed with JDK v1.8.0_74 {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 7m 20s
{color} | {color:green} trunk passed with JDK v1.7.0_95 {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
19s {color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 59s
{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
13s {color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m
35s {color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m 0s
{color} | {color:green} trunk passed with JDK v1.8.0_74 {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m 6s
{color} | {color:green} trunk passed with JDK v1.7.0_95 {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
43s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 8m 58s
{color} | {color:green} the patch passed with JDK v1.8.0_74 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 8m 58s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 6m 56s
{color} | {color:green} the patch passed with JDK v1.7.0_95 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 6m 56s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
18s {color} | {color:green} hadoop-common-project/hadoop-common: patch
generated 0 new + 12 unchanged - 5 fixed = 12 total (was 17) {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m 54s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
12s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s {color} | {color:green} Patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 1m
47s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m 59s
{color} | {color:green} the patch passed with JDK v1.8.0_74 {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 1m 16s
{color} | {color:green} the patch passed with JDK v1.7.0_95 {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 9m 45s {color}
| {color:red} hadoop-common in the patch failed with JDK v1.8.0_74. {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 9m 52s {color}
| {color:red} hadoop-common in the patch failed with JDK v1.7.0_95. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
23s {color} | {color:green} Patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 71m 36s {color}
| {color:black} {color} |
\\
\\
|| Reason || Tests ||
| JDK v1.8.0_74 Failed junit tests | hadoop.ha.TestZKFailoverController |
| JDK v1.7.0_95 Failed junit tests | hadoop.ha.TestZKFailoverController |
| | hadoop.security.ssl.TestReloadingX509TrustManager |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:0ca8df7 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12792477/HADOOP-12912.001.patch
|
| JIRA Issue | HADOOP-12912 |
| Optional Tests | asflicense
[jira] [Commented] (HADOOP-12912) Add LOG.isDebugEnabled() guard in Progress.set method
[
https://issues.apache.org/jira/browse/HADOOP-12912?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15189009#comment-15189009
]
Akira AJISAKA commented on HADOOP-12912:
We are removing unnecessarily guarding from Hadoop source-tree by moving to
slf4j, so generally adding guarding seems not to be a good idea. However, if
{{Progress.set(float progress)}} is called from hot path and the {{progress}}
is not in \[0, 1\], there are many String instance creations and the cost
becomes higher.
Therefore I'm +1 for adding guards. Would you add a comment that the method is
called from hot path and that's why we need guarding to save the cost of
creating String instances?
> Add LOG.isDebugEnabled() guard in Progress.set method
> -
>
> Key: HADOOP-12912
> URL: https://issues.apache.org/jira/browse/HADOOP-12912
> Project: Hadoop Common
> Issue Type: Bug
>Reporter: Tsuyoshi Ozawa
>Assignee: Tsuyoshi Ozawa
> Attachments: HADOOP-12912.001.patch
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (HADOOP-12912) Add LOG.isDebugEnabled() guard in Progress.set method
[ https://issues.apache.org/jira/browse/HADOOP-12912?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15188946#comment-15188946 ] Tsuyoshi Ozawa commented on HADOOP-12912: - TEZ-2215 reported this since it can be called from hot path(MergeQueue.next). > Add LOG.isDebugEnabled() guard in Progress.set method > - > > Key: HADOOP-12912 > URL: https://issues.apache.org/jira/browse/HADOOP-12912 > Project: Hadoop Common > Issue Type: Bug >Reporter: Tsuyoshi Ozawa > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
