[jira] [Commented] (HDFS-10752) Several log refactoring/improvement suggestion in HDFS
[
https://issues.apache.org/jira/browse/HDFS-10752?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15590371#comment-15590371
]
Hudson commented on HDFS-10752:
---
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10639 (See
[https://builds.apache.org/job/Hadoop-trunk-Commit/10639/])
HDFS-10752. Several log refactoring/improvement suggestion in HDFS. (arp: rev
b4564103e4709caa1135f6ccc2864d90e54f2ac9)
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
* (edit)
hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/OpenFileCtxCache.java
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java
> Several log refactoring/improvement suggestion in HDFS
> --
>
> Key: HDFS-10752
> URL: https://issues.apache.org/jira/browse/HDFS-10752
> Project: Hadoop HDFS
> Issue Type: Bug
>Affects Versions: 2.7.2
>Reporter: Nemo Chen
>Assignee: Hanisha Koneru
> Labels: easyfix, easytest
> Fix For: 2.8.0, 3.0.0-alpha2
>
> Attachments: HDFS-10752.000.patch
>
>
> As per conversation with [~vrushalic], we merged HDFS-10749, HDFS-10750,
> HDFS-10751, HDFS-10753, under this issue.
>
> HDFS-10749
> *Method invocation in logs can be replaced by variable*
> Similar to the fix for HDFS-409. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
> In code block:
> {code:borderStyle=solid}
> lastQueuedSeqno = currentPacket.getSeqno();
> if (DFSClient.LOG.isDebugEnabled()) {
> DFSClient.LOG.debug("Queued packet " + currentPacket.getSeqno());
> }
> {code}
> currentPacket.getSeqno() is better to be replaced by variable lastQueuedSeqno.
>
> HDFS-10750
> Similar to the fix for AVRO-115. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
> in line 695, the logging code:
> {code:borderStyle=solid}
> LOG.info(getRole() + " RPC up at: " + rpcServer.getRpcAddress());
> {code}
> In the same class, there is a method in line 907:
> {code:borderStyle=solid}
> /**
>* @return NameNode RPC address
>*/
> public InetSocketAddress getNameNodeAddress() {
> return rpcServer.getRpcAddress();
> }
> {code}
> We can tell that rpcServer.getRpcAddress() could be replaced by method
> getNameNodeAddress() for the case of readability and simplicity
>
> HDFS-10751
> Similar to the fix for AVRO-115. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/OpenFileCtxCache.java
> in line 72, the logging code:
> {code:borderStyle=solid}
> LOG.trace("openFileMap size:" + openFileMap.size());
> {code}
> In the same class, there is a method in line 189:
> {code:borderStyle=solid}
> int size() {
> return openFileMap.size();
> }
> {code}
> We can tell that openFileMap.size() could be replaced by method size() for
> the case of readability and simplicity
>
> *Print variable in byte*
> Similar to the fix for HBASE-623, in file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java
> In the following method, the log printed variable data (in byte[]). A
> possible fix is add Bytes.toString(data).
> {code}
> /**
>* Write the batch of edits to the local copy of the edit logs.
>*/
> private void logEditsLocally(long firstTxId, int numTxns, byte[] data) {
> long expectedTxId = editLog.getLastWrittenTxId() + 1;
> Preconditions.checkState(firstTxId == expectedTxId,
> "received txid batch starting at %s but expected txn %s",
> firstTxId, expectedTxId);
> editLog.setNextTxId(firstTxId + numTxns - 1);
> editLog.logEdit(data.length, data);
> editLog.logSync();
> }
> {code}
>
>
> HDFS-10753
> *MethodInvocation replaced by variable due to toString method*
> Similar to the fix in HADOOP-6419, in file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java
> in line 76, the blk.getBlockName() method invocation is invoked on variable
> blk. "blk" is the class instance of Block.
> {code}
> void addToCorruptReplicasMap(Block blk, DatanodeDescriptor dn,
> String reason, Reason reasonCode) {
> ...
> NameNode.blockStateChangeLog.info(
> "BLOCK NameSystem.addToCorruptReplicasMap: {} added as corrupt on "
> + "{} by {} {}", blk.getBlockName(), dn, Server.getRemoteIp(),
> reasonText);
> {code}
> In file:
>
[jira] [Commented] (HDFS-10752) Several log refactoring/improvement suggestion in HDFS
[
https://issues.apache.org/jira/browse/HDFS-10752?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15586162#comment-15586162
]
Arpit Agarwal commented on HDFS-10752:
--
So the log guard is not necessary after all.
+1, I will commit this later today.
> Several log refactoring/improvement suggestion in HDFS
> --
>
> Key: HDFS-10752
> URL: https://issues.apache.org/jira/browse/HDFS-10752
> Project: Hadoop HDFS
> Issue Type: Bug
>Affects Versions: 2.7.2
>Reporter: Nemo Chen
>Assignee: Hanisha Koneru
> Labels: easyfix, easytest
> Attachments: HDFS-10752.000.patch
>
>
> As per conversation with [~vrushalic], we merged HDFS-10749, HDFS-10750,
> HDFS-10751, HDFS-10753, under this issue.
>
> HDFS-10749
> *Method invocation in logs can be replaced by variable*
> Similar to the fix for HDFS-409. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
> In code block:
> {code:borderStyle=solid}
> lastQueuedSeqno = currentPacket.getSeqno();
> if (DFSClient.LOG.isDebugEnabled()) {
> DFSClient.LOG.debug("Queued packet " + currentPacket.getSeqno());
> }
> {code}
> currentPacket.getSeqno() is better to be replaced by variable lastQueuedSeqno.
>
> HDFS-10750
> Similar to the fix for AVRO-115. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
> in line 695, the logging code:
> {code:borderStyle=solid}
> LOG.info(getRole() + " RPC up at: " + rpcServer.getRpcAddress());
> {code}
> In the same class, there is a method in line 907:
> {code:borderStyle=solid}
> /**
>* @return NameNode RPC address
>*/
> public InetSocketAddress getNameNodeAddress() {
> return rpcServer.getRpcAddress();
> }
> {code}
> We can tell that rpcServer.getRpcAddress() could be replaced by method
> getNameNodeAddress() for the case of readability and simplicity
>
> HDFS-10751
> Similar to the fix for AVRO-115. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/OpenFileCtxCache.java
> in line 72, the logging code:
> {code:borderStyle=solid}
> LOG.trace("openFileMap size:" + openFileMap.size());
> {code}
> In the same class, there is a method in line 189:
> {code:borderStyle=solid}
> int size() {
> return openFileMap.size();
> }
> {code}
> We can tell that openFileMap.size() could be replaced by method size() for
> the case of readability and simplicity
>
> *Print variable in byte*
> Similar to the fix for HBASE-623, in file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java
> In the following method, the log printed variable data (in byte[]). A
> possible fix is add Bytes.toString(data).
> {code}
> /**
>* Write the batch of edits to the local copy of the edit logs.
>*/
> private void logEditsLocally(long firstTxId, int numTxns, byte[] data) {
> long expectedTxId = editLog.getLastWrittenTxId() + 1;
> Preconditions.checkState(firstTxId == expectedTxId,
> "received txid batch starting at %s but expected txn %s",
> firstTxId, expectedTxId);
> editLog.setNextTxId(firstTxId + numTxns - 1);
> editLog.logEdit(data.length, data);
> editLog.logSync();
> }
> {code}
>
>
> HDFS-10753
> *MethodInvocation replaced by variable due to toString method*
> Similar to the fix in HADOOP-6419, in file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java
> in line 76, the blk.getBlockName() method invocation is invoked on variable
> blk. "blk" is the class instance of Block.
> {code}
> void addToCorruptReplicasMap(Block blk, DatanodeDescriptor dn,
> String reason, Reason reasonCode) {
> ...
> NameNode.blockStateChangeLog.info(
> "BLOCK NameSystem.addToCorruptReplicasMap: {} added as corrupt on "
> + "{} by {} {}", blk.getBlockName(), dn, Server.getRemoteIp(),
> reasonText);
> {code}
> In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java
> {code}
> @Override
> public String toString() {
> return getBlockName() + "_" + getGenerationStamp();
> }
> {code}
> The toString() method contain not only getBlockName() but also
> getGenerationStamp which may be helpful for debugging purpose. Therefore
> blk.getBlockName() can be replaced by blk
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
-
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional
[jira] [Commented] (HDFS-10752) Several log refactoring/improvement suggestion in HDFS
[
https://issues.apache.org/jira/browse/HDFS-10752?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15586025#comment-15586025
]
Arpit Agarwal commented on HDFS-10752:
--
Thanks for the contribution [~hanishakoneru].
I think we should skip the change in {{addToCorruptReplicasMap}}. Some of the
overrides of {{Block#toString()}} are expensive. Or at least we should surround
the Log call with logging guards like so the overhead of {{Block#toString()}}
is avoided when debug logging is off.
if (NameNode.blockStateChangeLog.isDebugEnabled) {
NameNode.blockStateChangeLog.debug(...
}
+1 for the rest of the changes.
> Several log refactoring/improvement suggestion in HDFS
> --
>
> Key: HDFS-10752
> URL: https://issues.apache.org/jira/browse/HDFS-10752
> Project: Hadoop HDFS
> Issue Type: Bug
>Affects Versions: 2.7.2
>Reporter: Nemo Chen
>Assignee: Hanisha Koneru
> Labels: easyfix, easytest
> Attachments: HDFS-10752.000.patch
>
>
> As per conversation with [~vrushalic], we merged HDFS-10749, HDFS-10750,
> HDFS-10751, HDFS-10753, under this issue.
>
> HDFS-10749
> *Method invocation in logs can be replaced by variable*
> Similar to the fix for HDFS-409. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSOutputStream.java
> In code block:
> {code:borderStyle=solid}
> lastQueuedSeqno = currentPacket.getSeqno();
> if (DFSClient.LOG.isDebugEnabled()) {
> DFSClient.LOG.debug("Queued packet " + currentPacket.getSeqno());
> }
> {code}
> currentPacket.getSeqno() is better to be replaced by variable lastQueuedSeqno.
>
> HDFS-10750
> Similar to the fix for AVRO-115. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
> in line 695, the logging code:
> {code:borderStyle=solid}
> LOG.info(getRole() + " RPC up at: " + rpcServer.getRpcAddress());
> {code}
> In the same class, there is a method in line 907:
> {code:borderStyle=solid}
> /**
>* @return NameNode RPC address
>*/
> public InetSocketAddress getNameNodeAddress() {
> return rpcServer.getRpcAddress();
> }
> {code}
> We can tell that rpcServer.getRpcAddress() could be replaced by method
> getNameNodeAddress() for the case of readability and simplicity
>
> HDFS-10751
> Similar to the fix for AVRO-115. In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/OpenFileCtxCache.java
> in line 72, the logging code:
> {code:borderStyle=solid}
> LOG.trace("openFileMap size:" + openFileMap.size());
> {code}
> In the same class, there is a method in line 189:
> {code:borderStyle=solid}
> int size() {
> return openFileMap.size();
> }
> {code}
> We can tell that openFileMap.size() could be replaced by method size() for
> the case of readability and simplicity
>
> *Print variable in byte*
> Similar to the fix for HBASE-623, in file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupImage.java
> In the following method, the log printed variable data (in byte[]). A
> possible fix is add Bytes.toString(data).
> {code}
> /**
>* Write the batch of edits to the local copy of the edit logs.
>*/
> private void logEditsLocally(long firstTxId, int numTxns, byte[] data) {
> long expectedTxId = editLog.getLastWrittenTxId() + 1;
> Preconditions.checkState(firstTxId == expectedTxId,
> "received txid batch starting at %s but expected txn %s",
> firstTxId, expectedTxId);
> editLog.setNextTxId(firstTxId + numTxns - 1);
> editLog.logEdit(data.length, data);
> editLog.logSync();
> }
> {code}
>
>
> HDFS-10753
> *MethodInvocation replaced by variable due to toString method*
> Similar to the fix in HADOOP-6419, in file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/CorruptReplicasMap.java
> in line 76, the blk.getBlockName() method invocation is invoked on variable
> blk. "blk" is the class instance of Block.
> {code}
> void addToCorruptReplicasMap(Block blk, DatanodeDescriptor dn,
> String reason, Reason reasonCode) {
> ...
> NameNode.blockStateChangeLog.info(
> "BLOCK NameSystem.addToCorruptReplicasMap: {} added as corrupt on "
> + "{} by {} {}", blk.getBlockName(), dn, Server.getRemoteIp(),
> reasonText);
> {code}
> In file:
> hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java
> {code}
> @Override
> public String toString() {
> return getBlockName() + "_" + getGenerationStamp();
> }
> {code}
> The toString() method
[jira] [Commented] (HDFS-10752) Several log refactoring/improvement suggestion in HDFS
[
https://issues.apache.org/jira/browse/HDFS-10752?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15583863#comment-15583863
]
Hadoop QA commented on HDFS-10752:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
18s{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:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
7s{color} | {color:blue} Maven dependency ordering for branch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 7m
0s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
28s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
34s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
17s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
27s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
22s{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
51s{color} | {color:green} trunk passed {color} |
| {color:blue}0{color} | {color:blue} mvndep {color} | {color:blue} 0m
6s{color} | {color:blue} Maven dependency ordering for patch {color} |
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 1m
0s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
16s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 1m
16s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
29s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 1m
3s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} mvneclipse {color} | {color:green} 0m
19s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} findbugs {color} | {color:green} 2m
18s{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
49s{color} | {color:green} the patch passed {color} |
| {color:red}-1{color} | {color:red} unit {color} | {color:red} 61m 36s{color}
| {color:red} hadoop-hdfs in the patch failed. {color} |
| {color:green}+1{color} | {color:green} unit {color} | {color:green} 1m
44s{color} | {color:green} hadoop-hdfs-nfs in the patch passed. {color} |
| {color:green}+1{color} | {color:green} asflicense {color} | {color:green} 0m
18s{color} | {color:green} The patch does not generate ASF License warnings.
{color} |
| {color:black}{color} | {color:black} {color} | {color:black} 86m 44s{color} |
{color:black} {color} |
\\
\\
|| Reason || Tests ||
| Failed junit tests | hadoop.hdfs.server.namenode.ha.TestHASafeMode |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Image:yetus/hadoop:9560f25 |
| JIRA Issue | HDFS-10752 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12833821/HDFS-10752.000.patch |
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite
unit findbugs checkstyle |
| uname | Linux 6f8d24ed1fa6 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed
Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh
|
| git revision | trunk / f5d9235 |
| Default Java | 1.8.0_101 |
| findbugs | v3.0.0 |
| unit |
https://builds.apache.org/job/PreCommit-HDFS-Build/17187/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
|
| Test Results |
https://builds.apache.org/job/PreCommit-HDFS-Build/17187/testReport/ |
| modules | C: hadoop-hdfs-project/hadoop-hdfs
hadoop-hdfs-project/hadoop-hdfs-nfs U: hadoop-hdfs-project |
| Console output |
