Uploaded image for project: 'Hadoop HDFS'
  1. Hadoop HDFS
  2. HDFS-10752

Several log refactoring/improvement suggestion in HDFS

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.2
    • Fix Version/s: 2.8.0, 3.0.0-alpha2
    • Component/s: None
    • Labels:
    • Hadoop Flags:
      Reviewed

      Description

      As per conversation with Vrushali C, 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:

      lastQueuedSeqno = currentPacket.getSeqno();
      if (DFSClient.LOG.isDebugEnabled()) {
          DFSClient.LOG.debug("Queued packet " + currentPacket.getSeqno());
      }
      

      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:

      LOG.info(getRole() + " RPC up at: " + rpcServer.getRpcAddress());
      

      In the same class, there is a method in line 907:

        /**
         * @return NameNode RPC address
         */
        public InetSocketAddress getNameNodeAddress() {
          return rpcServer.getRpcAddress();
        }
      

      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:

      LOG.trace("openFileMap size:" + openFileMap.size());
      

      In the same class, there is a method in line 189:

        int size() {
          return openFileMap.size();
        }
      

      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).

      /**
         * 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();
        }
      


      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.

      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);
      

      In file: hadoop-rel-release-2.7.2/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/Block.java

        @Override
        public String toString() {
          return getBlockName() + "_" + getGenerationStamp();
        }
      

      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

        Issue Links

          Activity

          Hide
          hanishakoneru Hanisha Koneru added a comment -
          • HDFS-10749 already resolved
          • Replaced rpcServer.getRpcAddress() with the method getNameNodeAddress() in NameNode.java
          • Replaced openFileMap.size() with the method size() in OpenFileCtxCache.java
          • Replaced blk.getBlockName() with the blk (toString()) in CorruptReplicasMap.java
          Show
          hanishakoneru Hanisha Koneru added a comment - HDFS-10749 already resolved Replaced rpcServer.getRpcAddress() with the method getNameNodeAddress() in NameNode.java Replaced openFileMap.size() with the method size() in OpenFileCtxCache.java Replaced blk.getBlockName() with the blk (toString()) in CorruptReplicasMap.java
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 18s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s 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.
          0 mvndep 0m 7s Maven dependency ordering for branch
          +1 mvninstall 7m 0s trunk passed
          +1 compile 1m 28s trunk passed
          +1 checkstyle 0m 34s trunk passed
          +1 mvnsite 1m 17s trunk passed
          +1 mvneclipse 0m 27s trunk passed
          +1 findbugs 2m 22s trunk passed
          +1 javadoc 0m 51s trunk passed
          0 mvndep 0m 6s Maven dependency ordering for patch
          +1 mvninstall 1m 0s the patch passed
          +1 compile 1m 16s the patch passed
          +1 javac 1m 16s the patch passed
          +1 checkstyle 0m 29s the patch passed
          +1 mvnsite 1m 3s the patch passed
          +1 mvneclipse 0m 19s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 2m 18s the patch passed
          +1 javadoc 0m 49s the patch passed
          -1 unit 61m 36s hadoop-hdfs in the patch failed.
          +1 unit 1m 44s hadoop-hdfs-nfs in the patch passed.
          +1 asflicense 0m 18s The patch does not generate ASF License warnings.
          86m 44s



          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 https://builds.apache.org/job/PreCommit-HDFS-Build/17187/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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. 0 mvndep 0m 7s Maven dependency ordering for branch +1 mvninstall 7m 0s trunk passed +1 compile 1m 28s trunk passed +1 checkstyle 0m 34s trunk passed +1 mvnsite 1m 17s trunk passed +1 mvneclipse 0m 27s trunk passed +1 findbugs 2m 22s trunk passed +1 javadoc 0m 51s trunk passed 0 mvndep 0m 6s Maven dependency ordering for patch +1 mvninstall 1m 0s the patch passed +1 compile 1m 16s the patch passed +1 javac 1m 16s the patch passed +1 checkstyle 0m 29s the patch passed +1 mvnsite 1m 3s the patch passed +1 mvneclipse 0m 19s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 2m 18s the patch passed +1 javadoc 0m 49s the patch passed -1 unit 61m 36s hadoop-hdfs in the patch failed. +1 unit 1m 44s hadoop-hdfs-nfs in the patch passed. +1 asflicense 0m 18s The patch does not generate ASF License warnings. 86m 44s 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 https://builds.apache.org/job/PreCommit-HDFS-Build/17187/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          arpitagarwal Arpit Agarwal added a comment - - edited

          Thanks for the contribution Hanisha Koneru.

          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.

          Show
          arpitagarwal Arpit Agarwal added a comment - - edited Thanks for the contribution Hanisha Koneru . 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.
          Hide
          arpitagarwal Arpit Agarwal added a comment -

          So the log guard is not necessary after all.

          +1, I will commit this later today.

          Show
          arpitagarwal Arpit Agarwal added a comment - So the log guard is not necessary after all. +1, I will commit this later today.
          Hide
          arpitagarwal Arpit Agarwal added a comment -

          Committed for 2.8.0. Thanks for the contribution Hanisha Koneru!

          Show
          arpitagarwal Arpit Agarwal added a comment - Committed for 2.8.0. Thanks for the contribution Hanisha Koneru !
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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

            People

            • Assignee:
              hanishakoneru Hanisha Koneru
              Reporter:
              chenfsd Nemo Chen
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development