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

HDFS snapshots doesn't capture all open files when one of the open files is deleted

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha1
    • Fix Version/s: 2.9.0, 3.0.0-beta1
    • Component/s: snapshots
    • Labels:
      None

      Description

      With the fix for HDFS-11402, HDFS Snapshots can additionally capture all the open files. Just like all other files, these open files in the snapshots will remain immutable. But, sometimes it is found that snapshots fail to capture all the open files in the system.

      Under the following conditions, LeaseManager will fail to find INode corresponding to an active lease

      • a file is opened for writing (LeaseManager allots a lease), and
      • the same file is deleted while it is still open for writing and having active lease, and
      • the same file is not referenced in any other Snapshots/Trash

      INode[] LeaseManager#getINodesWithLease() can thus return null for few leases there by causing the caller to trip over and not return all the open files needed by the snapshot manager.

      1. HDFS-12217.01.patch
        9 kB
        Manoj Govindassamy
      2. HDFS-12217.02.patch
        11 kB
        Manoj Govindassamy
      3. HDFS-12217.03.patch
        11 kB
        Manoj Govindassamy
      4. HDFS-12217.04.patch
        11 kB
        Manoj Govindassamy
      5. HDFS-12217.05.patch
        12 kB
        Manoj Govindassamy

        Issue Links

          Activity

          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Manoj Govindassamy and Wei-Chiu Chuang,

          Thanks for continuing to drive on this when I was out. I am glad to see that we have converged on the solution.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Manoj Govindassamy and Wei-Chiu Chuang , Thanks for continuing to drive on this when I was out. I am glad to see that we have converged on the solution.
          Hide
          manojg Manoj Govindassamy added a comment -

          Resolved the issue in TestLeaseManager and verified the successful run. Committed to branch-2. Thanks Jason Lowe.

          Show
          manojg Manoj Govindassamy added a comment - Resolved the issue in TestLeaseManager and verified the successful run. Committed to branch-2. Thanks Jason Lowe .
          Hide
          manojg Manoj Govindassamy added a comment -

          Sorry for the inconvenience Jason Lowe. The cherrypick from the trunk and the follow on build went through smoothly and I missed the test issue. Will rectify the error. Thanks for the help.

          Show
          manojg Manoj Govindassamy added a comment - Sorry for the inconvenience Jason Lowe . The cherrypick from the trunk and the follow on build went through smoothly and I missed the test issue. Will rectify the error. Thanks for the help.
          Hide
          jlowe Jason Lowe added a comment -

          I reverted this from branch-2 because the commit broke the build.

          [ERROR] COMPILATION ERROR : 
          [INFO] -------------------------------------------------------------
          [ERROR] /home/jlowe/hadoop/apache/hadoop/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestLeaseManager.java:[395,40] unreported exception java.io.IOException; must be caught or declared to be thrown
          [ERROR] /home/jlowe/hadoop/apache/hadoop/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestLeaseManager.java:[397,40] unreported exception java.io.IOException; must be caught or declared to be thrown
          [INFO] 2 errors 
          

          The patch will need to be updated for branch-2 and recommitted.

          Show
          jlowe Jason Lowe added a comment - I reverted this from branch-2 because the commit broke the build. [ERROR] COMPILATION ERROR : [INFO] ------------------------------------------------------------- [ERROR] /home/jlowe/hadoop/apache/hadoop/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestLeaseManager.java:[395,40] unreported exception java.io.IOException; must be caught or declared to be thrown [ERROR] /home/jlowe/hadoop/apache/hadoop/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestLeaseManager.java:[397,40] unreported exception java.io.IOException; must be caught or declared to be thrown [INFO] 2 errors The patch will need to be updated for branch-2 and recommitted.
          Hide
          manojg Manoj Govindassamy added a comment -

          Committed to trunk and branch-2.
          Thanks for the review Wei-Chiu Chuang and Yongjun Zhang.

          Show
          manojg Manoj Govindassamy added a comment - Committed to trunk and branch-2. Thanks for the review Wei-Chiu Chuang and Yongjun Zhang .
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12096 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12096/)
          HDFS-12217. HDFS snapshots doesn't capture all open files when one of (manojpec: rev 52d7bafcf49916887197436ddb0f08f021d248d9)

          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/SnapshotException.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestOpenFilesWithSnapshot.java
          • (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12096 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12096/ ) HDFS-12217 . HDFS snapshots doesn't capture all open files when one of (manojpec: rev 52d7bafcf49916887197436ddb0f08f021d248d9) (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/LeaseManager.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/SnapshotException.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/snapshot/DirectorySnapshottableFeature.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/snapshot/TestOpenFilesWithSnapshot.java (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
          Hide
          manojg Manoj Govindassamy added a comment -

          Unit test failure and the findbug warnings are not related the patch.

          Show
          manojg Manoj Govindassamy added a comment - Unit test failure and the findbug warnings are not related the patch.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 13m 15s trunk passed
          +1 compile 0m 47s trunk passed
          +1 checkstyle 0m 37s trunk passed
          +1 mvnsite 0m 54s trunk passed
          -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings.
          +1 javadoc 0m 41s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 48s the patch passed
          +1 compile 0m 44s the patch passed
          +1 javac 0m 44s the patch passed
          +1 checkstyle 0m 36s the patch passed
          +1 mvnsite 0m 50s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 45s the patch passed
          +1 javadoc 0m 36s the patch passed
                Other Tests
          -1 unit 63m 56s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 15s The patch does not generate ASF License warnings.
          88m 53s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HDFS-12217
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879864/HDFS-12217.05.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux a05cc83d7ed2 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / b38a1ee
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20517/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/20517/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20517/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20517/console
          Powered by Apache Yetus 0.6.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 13s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 15s trunk passed +1 compile 0m 47s trunk passed +1 checkstyle 0m 37s trunk passed +1 mvnsite 0m 54s trunk passed -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. +1 javadoc 0m 41s trunk passed       Patch Compile Tests +1 mvninstall 0m 48s the patch passed +1 compile 0m 44s the patch passed +1 javac 0m 44s the patch passed +1 checkstyle 0m 36s the patch passed +1 mvnsite 0m 50s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 45s the patch passed +1 javadoc 0m 36s the patch passed       Other Tests -1 unit 63m 56s hadoop-hdfs in the patch failed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 88m 53s Reason Tests Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150 Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-12217 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879864/HDFS-12217.05.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux a05cc83d7ed2 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / b38a1ee Default Java 1.8.0_131 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20517/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/20517/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20517/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20517/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          +1 pending Jenkins.

          Show
          jojochuang Wei-Chiu Chuang added a comment - +1 pending Jenkins.
          Hide
          manojg Manoj Govindassamy added a comment -

          Thanks for the review Wei-Chiu Chuang. Attached v05 patch with following comment addressed. Please take a look.

          DirectorySnapshottableFeature#addSnapshot, because an exception could be thrown from places other than LeaseManager#getINodeWithLeases

          Sure. Modified the SnapshotException to accept both message and throwable. This way, the exception is printed is the full chain instead of the top one. This exception is also get sent to the Client. When the DEUBG logging is enabled, the client command can also see the full error exception stack tracer.

          Show
          manojg Manoj Govindassamy added a comment - Thanks for the review Wei-Chiu Chuang . Attached v05 patch with following comment addressed. Please take a look. DirectorySnapshottableFeature#addSnapshot, because an exception could be thrown from places other than LeaseManager#getINodeWithLeases Sure. Modified the SnapshotException to accept both message and throwable. This way, the exception is printed is the full chain instead of the top one. This exception is also get sent to the Client. When the DEUBG logging is enabled, the client command can also see the full error exception stack tracer.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Thanks Manoj Govindassamy for the latest patch.
          I am still a little concerned not logging the exception in DirectorySnapshottableFeature#addSnapshot, because an exception could be thrown from places other than LeaseManager#getINodeWithLeases (such as an NPE in INodesInPath#getLastINode). If that happens, logging the exception message wouldn't help much.

          Can we add an overloaded SnapshotException constructor that has an additional Throwable as an extra parameter, and throw that instead so that we pass that exception to the client?

          Other than this, the rest LGTM.

          Thanks.

          Show
          jojochuang Wei-Chiu Chuang added a comment - Thanks Manoj Govindassamy for the latest patch. I am still a little concerned not logging the exception in DirectorySnapshottableFeature#addSnapshot , because an exception could be thrown from places other than LeaseManager#getINodeWithLeases (such as an NPE in INodesInPath#getLastINode). If that happens, logging the exception message wouldn't help much. Can we add an overloaded SnapshotException constructor that has an additional Throwable as an extra parameter, and throw that instead so that we pass that exception to the client? Other than this, the rest LGTM. Thanks.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 16s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 13m 33s trunk passed
          +1 compile 0m 50s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 0m 54s trunk passed
          -1 findbugs 1m 40s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings.
          +1 javadoc 0m 40s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 48s the patch passed
          +1 compile 0m 47s the patch passed
          +1 javac 0m 47s the patch passed
          +1 checkstyle 0m 36s the patch passed
          +1 mvnsite 0m 53s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 47s the patch passed
          +1 javadoc 0m 38s the patch passed
                Other Tests
          -1 unit 67m 48s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 17s The patch does not generate ASF License warnings.
          93m 21s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure140
            hadoop.hdfs.TestDFSStripedInputStreamWithRandomECPolicy
            hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HDFS-12217
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879752/HDFS-12217.04.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 425425fe3a9a 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / ea56812
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20507/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/20507/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20507/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20507/console
          Powered by Apache Yetus 0.6.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 16s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 33s trunk passed +1 compile 0m 50s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 0m 54s trunk passed -1 findbugs 1m 40s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. +1 javadoc 0m 40s trunk passed       Patch Compile Tests +1 mvninstall 0m 48s the patch passed +1 compile 0m 47s the patch passed +1 javac 0m 47s the patch passed +1 checkstyle 0m 36s the patch passed +1 mvnsite 0m 53s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 47s the patch passed +1 javadoc 0m 38s the patch passed       Other Tests -1 unit 67m 48s hadoop-hdfs in the patch failed. +1 asflicense 0m 17s The patch does not generate ASF License warnings. 93m 21s Reason Tests Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure140   hadoop.hdfs.TestDFSStripedInputStreamWithRandomECPolicy   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080 Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-12217 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879752/HDFS-12217.04.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 425425fe3a9a 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / ea56812 Default Java 1.8.0_131 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20507/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/20507/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20507/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20507/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          manojg Manoj Govindassamy added a comment -

          Thanks for the quick review Wei-Chiu Chuang. Attached v04 patch to address the following. Can you please take a look at the latest patch.

          LeaseManager#getINodeWithLeases() seems to be a test-only method. If so, it doesn't need public modifier (package private is sufficient) and we can also add a {{@VisibleForTesting }} annotation.

          Done.

          On a separate note, and this is totally unrelated to this patch. It looks like LeaseManager#addLease assumes the inodeId is an id for an INodeFile, which makes perfect sense.

          Will take this up in a new patch as there are many callers that need to get refactored for this,.

          Note that in your patch, DirectorySnapshottableFeature#addSnapshot would capture exception, but doesn't log the exception. I also think that in addition to the snapshot name, you should print the snapshot root path for the error message.

          Done.
          – Exception is already logged at LeaseManager#getINodeWithLeases.
          – Now added exception details to the SnapshotException message also.

          Show
          manojg Manoj Govindassamy added a comment - Thanks for the quick review Wei-Chiu Chuang . Attached v04 patch to address the following. Can you please take a look at the latest patch. LeaseManager#getINodeWithLeases() seems to be a test-only method. If so, it doesn't need public modifier (package private is sufficient) and we can also add a {{@VisibleForTesting }} annotation. Done. On a separate note, and this is totally unrelated to this patch. It looks like LeaseManager#addLease assumes the inodeId is an id for an INodeFile, which makes perfect sense. Will take this up in a new patch as there are many callers that need to get refactored for this,. Note that in your patch, DirectorySnapshottableFeature#addSnapshot would capture exception, but doesn't log the exception. I also think that in addition to the snapshot name, you should print the snapshot root path for the error message. Done. – Exception is already logged at LeaseManager#getINodeWithLeases. – Now added exception details to the SnapshotException message also.
          Hide
          jojochuang Wei-Chiu Chuang added a comment -

          Thanks Manoj Govindassamy Yongjun Zhang.

          One nit I found is that LeaseManager#getINodeWithLeases() seems to be a test-only method. If so, it doesn't need public modifier (package private is sufficient) and we can also add a {{@VisibleForTesting }} annotation.

          On a separate note, and this is totally unrelated to this patch. It looks like LeaseManager#addLease assumes the inodeId is an id for an INodeFile, which makes perfect sense. The member variable leasesById also implicitly makes the assumption that the id is for an INodeFile. However, there's no assertion to ensure it is the case in the future. I wonder if it makes sense to add a new addLease(String, INodeFile) method to make sure only INodeFile is passed in. So it would go like:

          Lease addLease(String holder, INodeFile inodeFile) {
            addLease(holder, inodeFile.getId());
          }
          private synchronized Lease addLease(String holder, long inodeId) {
           ...
          }
          

          Note that in your patch, DirectorySnapshottableFeature#addSnapshot would capture exception, but doesn't log the exception. I also think that in addition to the snapshot name, you should print the snapshot root path for the error message.

          Show
          jojochuang Wei-Chiu Chuang added a comment - Thanks Manoj Govindassamy Yongjun Zhang . One nit I found is that LeaseManager#getINodeWithLeases() seems to be a test-only method. If so, it doesn't need public modifier (package private is sufficient) and we can also add a {{@VisibleForTesting }} annotation. On a separate note, and this is totally unrelated to this patch. It looks like LeaseManager#addLease assumes the inodeId is an id for an INodeFile, which makes perfect sense. The member variable leasesById also implicitly makes the assumption that the id is for an INodeFile. However, there's no assertion to ensure it is the case in the future. I wonder if it makes sense to add a new addLease(String, INodeFile) method to make sure only INodeFile is passed in. So it would go like: Lease addLease( String holder, INodeFile inodeFile) { addLease(holder, inodeFile.getId()); } private synchronized Lease addLease( String holder, long inodeId) { ... } Note that in your patch, DirectorySnapshottableFeature#addSnapshot would capture exception, but doesn't log the exception. I also think that in addition to the snapshot name, you should print the snapshot root path for the error message.
          Hide
          manojg Manoj Govindassamy added a comment -

          Yongjun Zhang/Wei-Chiu Chuang,
          As per yongjun's suggestion, I modified SnapshotException to carry the throwable along with the message. Yet the shell command doesn't display the server exception stack trace as expected. As we discussed previously we can take on this common shell-command error reporting model in a different jira. Will wait for some more time for others to comment on patch v03. Thanks.

          Show
          manojg Manoj Govindassamy added a comment - Yongjun Zhang / Wei-Chiu Chuang , As per yongjun's suggestion, I modified SnapshotException to carry the throwable along with the message. Yet the shell command doesn't display the server exception stack trace as expected. As we discussed previously we can take on this common shell-command error reporting model in a different jira. Will wait for some more time for others to comment on patch v03. Thanks.
          Hide
          manojg Manoj Govindassamy added a comment -

          Thanks for the details Yongjun Zhang.
          As you are aware, the NameNode is already logging all the exception and stack traces with clear messages. The one pending item we are discussing here is, "createSnapshot" shell command to display the long NameNode error stack traces in the console along with the error message. I personally prefer the client commands to show only meaningful detailed error messages and not the long error stack traces. But heard from you that the general model in the hadoop shell commands is to display the stack trace also. I tried the SnapshotException(string, throwable) approach, and still the client shell command doesn't see the stack trace. It only sees the error message. Can you please point me to the shell command where we display the error stack traces also to the console? I can try doing the same. Otherwise, as you recommended will take up this into a new jira and can get the others committed. Thanks for the review.

          Show
          manojg Manoj Govindassamy added a comment - Thanks for the details Yongjun Zhang . As you are aware, the NameNode is already logging all the exception and stack traces with clear messages. The one pending item we are discussing here is, "createSnapshot" shell command to display the long NameNode error stack traces in the console along with the error message. I personally prefer the client commands to show only meaningful detailed error messages and not the long error stack traces. But heard from you that the general model in the hadoop shell commands is to display the stack trace also. I tried the SnapshotException(string, throwable) approach, and still the client shell command doesn't see the stack trace. It only sees the error message. Can you please point me to the shell command where we display the error stack traces also to the console? I can try doing the same. Otherwise, as you recommended will take up this into a new jira and can get the others committed. Thanks for the review.
          Hide
          yzhangal Yongjun Zhang added a comment - - edited

          Hi Manoj Govindassamy,

          LeaseManager#getINodeWithLeases already logs the warning message with full exception and stack trace.

          The logging you mentioned above happens at the server side and is about the cause of the failure. What I hope to see is the exception chain at client side when SnapshotException is thrown, so user can see immediately what caused the exception in one place. Collecting the server side log to match to the client symptom is a support effort which I hope we can avoid if possible. Another thing is, server side log can be rolled too.

          Currently SnapshotException is:

          public class SnapshotException extends IOException {
            private static final long serialVersionUID = 1L;
          
            public SnapshotException(final String message) {
              super(message);
            }
          
            public SnapshotException(final Throwable cause) {
              super(cause);
            }
          }
          

          I examined all the places that call SnapshotException(final String message), none of them is called inside exception catch block. So the patch here is the first time that we are throwing SnapshotException inside an exception catch block. Interestingly, I don't see any places using SnapshotException(final Throwable cause). So it seems introducing a method public SnapshotException(String message, Throwable cause) and use it in this patch will not introduce inconsistency because it will be the first use of it.

          That's what I preferred. However, if you like to create a new jira to add the new API, and change the single use which is introduced by the patch here, It's ok with me too.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - - edited Hi Manoj Govindassamy , LeaseManager#getINodeWithLeases already logs the warning message with full exception and stack trace. The logging you mentioned above happens at the server side and is about the cause of the failure. What I hope to see is the exception chain at client side when SnapshotException is thrown, so user can see immediately what caused the exception in one place. Collecting the server side log to match to the client symptom is a support effort which I hope we can avoid if possible. Another thing is, server side log can be rolled too. Currently SnapshotException is: public class SnapshotException extends IOException { private static final long serialVersionUID = 1L; public SnapshotException( final String message) { super (message); } public SnapshotException( final Throwable cause) { super (cause); } } I examined all the places that call SnapshotException(final String message) , none of them is called inside exception catch block. So the patch here is the first time that we are throwing SnapshotException inside an exception catch block. Interestingly, I don't see any places using SnapshotException(final Throwable cause) . So it seems introducing a method public SnapshotException(String message, Throwable cause) and use it in this patch will not introduce inconsistency because it will be the first use of it. That's what I preferred. However, if you like to create a new jira to add the new API, and change the single use which is introduced by the patch here, It's ok with me too. Thanks.
          Hide
          manojg Manoj Govindassamy added a comment -
          • LeaseManager#getINodeWithLeases already logs the warning message with full exception and stack trace.
          • The caller wraps the message into a SnapshotException for the upper layer usage.
          • SnapshotException() doesn't accept the throwable argument. We need to change SnapshotException() and all the users in the all the snapshot commands to have an uniform behavior of returning the throwable in the RPC call. Will file a new jira for this.
            Yongjun Zhang, are you ok with this ?
          Show
          manojg Manoj Govindassamy added a comment - LeaseManager#getINodeWithLeases already logs the warning message with full exception and stack trace. The caller wraps the message into a SnapshotException for the upper layer usage. SnapshotException() doesn't accept the throwable argument. We need to change SnapshotException() and all the users in the all the snapshot commands to have an uniform behavior of returning the throwable in the RPC call. Will file a new jira for this. Yongjun Zhang , are you ok with this ?
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks for the new rev Manoj Govindassamy.

          When a user see the failure, s/he might want to know more about what's the root cause. Though some error is logged at server side, server side log may not be immediately available. And it would be nice to see an cause-effect chain log visible to user on the spot. This will reduce support effort. Agree that there will be similar log at both server and client, but I think it doesn't really hurt. So I still prefer having the e put into the cause of the exception. Hope this makes sense to you.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks for the new rev Manoj Govindassamy . When a user see the failure, s/he might want to know more about what's the root cause. Though some error is logged at server side, server side log may not be immediately available. And it would be nice to see an cause-effect chain log visible to user on the spot. This will reduce support effort. Agree that there will be similar log at both server and client, but I think it doesn't really hurt. So I still prefer having the e put into the cause of the exception. Hope this makes sense to you. Thanks.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 12s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 13m 21s trunk passed
          +1 compile 0m 51s trunk passed
          +1 checkstyle 0m 39s trunk passed
          +1 mvnsite 1m 0s trunk passed
          -1 findbugs 1m 46s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings.
          +1 javadoc 0m 41s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 48s the patch passed
          +1 compile 0m 45s the patch passed
          +1 javac 0m 45s the patch passed
          +1 checkstyle 0m 35s the patch passed
          +1 mvnsite 0m 50s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 44s the patch passed
          +1 javadoc 0m 37s the patch passed
                Other Tests
          +1 unit 64m 7s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          89m 27s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HDFS-12217
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879473/HDFS-12217.03.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux d793814cb949 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 890e14c
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20484/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20484/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20484/console
          Powered by Apache Yetus 0.6.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 12s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 21s trunk passed +1 compile 0m 51s trunk passed +1 checkstyle 0m 39s trunk passed +1 mvnsite 1m 0s trunk passed -1 findbugs 1m 46s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. +1 javadoc 0m 41s trunk passed       Patch Compile Tests +1 mvninstall 0m 48s the patch passed +1 compile 0m 45s the patch passed +1 javac 0m 45s the patch passed +1 checkstyle 0m 35s the patch passed +1 mvnsite 0m 50s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 44s the patch passed +1 javadoc 0m 37s the patch passed       Other Tests +1 unit 64m 7s hadoop-hdfs in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 89m 27s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-12217 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879473/HDFS-12217.03.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d793814cb949 3.13.0-119-generic #166-Ubuntu SMP Wed May 3 12:18:55 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 890e14c Default Java 1.8.0_131 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20484/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20484/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20484/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          manojg Manoj Govindassamy added a comment -

          Thanks for the quick review Yongjun Zhang. Attached v03 patch with additional logging.

          Suggest to put the e as the cause in the exception throwing.

          The SnasphotException message goes to the user. It already has the high level details on what went wrong. Adding IOException details to the client shell command output might not look good. On the server side, the RPC server handler already logs the exception. Additionally, now I have added the throwable details in the warning log at LeaseManager. These details are good for debugging if any problem arises.

          # hdfs dfs -createSnapshot /sub1 sn0
          Failed to add snapshot: Unable to capture all open files for the snapshot 'sn0'.
          
          Show
          manojg Manoj Govindassamy added a comment - Thanks for the quick review Yongjun Zhang . Attached v03 patch with additional logging. Suggest to put the e as the cause in the exception throwing. The SnasphotException message goes to the user. It already has the high level details on what went wrong. Adding IOException details to the client shell command output might not look good. On the server side, the RPC server handler already logs the exception. Additionally, now I have added the throwable details in the warning log at LeaseManager. These details are good for debugging if any problem arises. # hdfs dfs -createSnapshot /sub1 sn0 Failed to add snapshot: Unable to capture all open files for the snapshot 'sn0'.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 14s Docker mode activated.
                Prechecks
          +1 @author 0m 1s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 13m 32s trunk passed
          +1 compile 0m 52s trunk passed
          +1 checkstyle 0m 39s trunk passed
          +1 mvnsite 0m 54s trunk passed
          -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings.
          +1 javadoc 0m 41s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 49s the patch passed
          +1 compile 0m 46s the patch passed
          +1 javac 0m 46s the patch passed
          +1 checkstyle 0m 36s the patch passed
          +1 mvnsite 0m 52s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 44s the patch passed
          +1 javadoc 0m 37s the patch passed
                Other Tests
          -1 unit 72m 57s hadoop-hdfs in the patch failed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          98m 24s



          Reason Tests
          Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150
            hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080
            hadoop.hdfs.TestDFSStripedOutputStreamWithFailure
            hadoop.hdfs.TestDFSStripedOutputStreamWithFailure070
            hadoop.hdfs.TestHDFSFileSystemContract
            hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HDFS-12217
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879458/HDFS-12217.02.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 71ca42c72c8c 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 890e14c
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20482/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          unit https://builds.apache.org/job/PreCommit-HDFS-Build/20482/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20482/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20482/console
          Powered by Apache Yetus 0.6.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 14s Docker mode activated.       Prechecks +1 @author 0m 1s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 32s trunk passed +1 compile 0m 52s trunk passed +1 checkstyle 0m 39s trunk passed +1 mvnsite 0m 54s trunk passed -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. +1 javadoc 0m 41s trunk passed       Patch Compile Tests +1 mvninstall 0m 49s the patch passed +1 compile 0m 46s the patch passed +1 javac 0m 46s the patch passed +1 checkstyle 0m 36s the patch passed +1 mvnsite 0m 52s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 44s the patch passed +1 javadoc 0m 37s the patch passed       Other Tests -1 unit 72m 57s hadoop-hdfs in the patch failed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 98m 24s Reason Tests Failed junit tests hadoop.hdfs.TestDFSStripedOutputStreamWithFailure150   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure080   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure   hadoop.hdfs.TestDFSStripedOutputStreamWithFailure070   hadoop.hdfs.TestHDFSFileSystemContract   hadoop.hdfs.server.datanode.TestDataNodeVolumeFailure Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-12217 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879458/HDFS-12217.02.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 71ca42c72c8c 3.13.0-116-generic #163-Ubuntu SMP Fri Mar 31 14:13:22 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 890e14c Default Java 1.8.0_131 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20482/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html unit https://builds.apache.org/job/PreCommit-HDFS-Build/20482/artifact/patchprocess/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20482/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20482/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Thanks for the new rev Manoj Govindassamy.

          Some comments:

          But, f.get() can be null if the task didn't get to run properly.

          if the task didn't run properly, f.get() would throw an exception instead of return null.

          Suggest to put the e as the cause in the exception throwing.

          205	      } catch (Exception e) {
          206	        throw new SnapshotException("Failed to add snapshot: Unable to " +
          207	            "capture all open files for the snapshot '" + name + "'.");
          

          +1 after that pending jenkins.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Thanks for the new rev Manoj Govindassamy . Some comments: But, f.get() can be null if the task didn't get to run properly. if the task didn't run properly, f.get() would throw an exception instead of return null. Suggest to put the e as the cause in the exception throwing. 205 } catch (Exception e) { 206 throw new SnapshotException( "Failed to add snapshot: Unable to " + 207 "capture all open files for the snapshot '" + name + "'." ); +1 after that pending jenkins. Thanks.
          Hide
          manojg Manoj Govindassamy added a comment -

          Thanks for the detailed review Yongjun Zhang. Attached v02 patch with following addressed. Please take a look at the latest patch.

          1. we can use package wide scope for the isFileDeleted method instead of protected

          Done.

          2. f can't be null in the code, I mean, either an exception will be thrown, or f will valid value.

          True. But, f.get() can be null if the task didn't get to run properly.
          Enclosed this code section in try{}catch{} block and now throwing IOException() on any errors.

          3. If any exception is thrown in the above code, we can log a waning, and rethrow the exception, because the exception likely indicate a real problem that we don't expect to happen. If we continue, we might get inconsistent snapshot.

          Makes sense. Now throwing the exception, addSnasphot() caller handles the exception and rethrows it as SnaphotException to the upper layer to abort the snapshot creation.

          Show
          manojg Manoj Govindassamy added a comment - Thanks for the detailed review Yongjun Zhang . Attached v02 patch with following addressed. Please take a look at the latest patch. 1. we can use package wide scope for the isFileDeleted method instead of protected Done. 2. f can't be null in the code, I mean, either an exception will be thrown, or f will valid value. True. But, f.get() can be null if the task didn't get to run properly. Enclosed this code section in try{}catch{} block and now throwing IOException() on any errors. 3. If any exception is thrown in the above code, we can log a waning, and rethrow the exception, because the exception likely indicate a real problem that we don't expect to happen. If we continue, we might get inconsistent snapshot. Makes sense. Now throwing the exception, addSnasphot() caller handles the exception and rethrows it as SnaphotException to the upper layer to abort the snapshot creation.
          Hide
          yzhangal Yongjun Zhang added a comment -

          Hi Manoj Govindassamy,

          Thanks for reporting the issue and working on.

          It looks good in general, some comments here:

          1. we can use package wide scope for the isFileDeleted method instead of protected

          2. f can't be null in the code, I mean, either an exception will be thrown, or f will valid value. So we don't need to check whether it's null here.

             for (Future<List<INodesInPath>> f : futureList) {
                try {
                  if (f != null) {
                    iipSet.addAll(f.get());
                  }
                } catch (Exception e) {
                  LOG.warn("INode filter task encountered exception: ", e);
                }
              }
          ...
          

          3. If any exception is thrown in the above code, we can log a waning, and rethrow the exception, because the exception likely indicate a real problem that we don't expect to happen. If we continue, we might get inconsistent snapshot.

          Thanks.

          Show
          yzhangal Yongjun Zhang added a comment - Hi Manoj Govindassamy , Thanks for reporting the issue and working on. It looks good in general, some comments here: 1. we can use package wide scope for the isFileDeleted method instead of protected 2. f can't be null in the code, I mean, either an exception will be thrown, or f will valid value. So we don't need to check whether it's null here. for (Future<List<INodesInPath>> f : futureList) { try { if (f != null ) { iipSet.addAll(f.get()); } } catch (Exception e) { LOG.warn( "INode filter task encountered exception: " , e); } } ... 3. If any exception is thrown in the above code, we can log a waning, and rethrow the exception, because the exception likely indicate a real problem that we don't expect to happen. If we continue, we might get inconsistent snapshot. Thanks.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 10s Docker mode activated.
                Prechecks
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
                trunk Compile Tests
          +1 mvninstall 13m 21s trunk passed
          +1 compile 0m 48s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 0m 52s trunk passed
          -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings.
          +1 javadoc 0m 41s trunk passed
                Patch Compile Tests
          +1 mvninstall 0m 48s the patch passed
          +1 compile 0m 46s the patch passed
          +1 javac 0m 46s the patch passed
          +1 checkstyle 0m 35s the patch passed
          +1 mvnsite 0m 52s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 44s the patch passed
          +1 javadoc 0m 37s the patch passed
                Other Tests
          +1 unit 64m 39s hadoop-hdfs in the patch passed.
          +1 asflicense 0m 16s The patch does not generate ASF License warnings.
          89m 41s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:14b5c93
          JIRA Issue HDFS-12217
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879424/HDFS-12217.01.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 070bf080bb55 3.13.0-123-generic #172-Ubuntu SMP Mon Jun 26 18:04:35 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 746189a
          Default Java 1.8.0_131
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20477/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html
          Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20477/testReport/
          modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
          Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20477/console
          Powered by Apache Yetus 0.6.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 10s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.       trunk Compile Tests +1 mvninstall 13m 21s trunk passed +1 compile 0m 48s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 0m 52s trunk passed -1 findbugs 1m 39s hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. +1 javadoc 0m 41s trunk passed       Patch Compile Tests +1 mvninstall 0m 48s the patch passed +1 compile 0m 46s the patch passed +1 javac 0m 46s the patch passed +1 checkstyle 0m 35s the patch passed +1 mvnsite 0m 52s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 44s the patch passed +1 javadoc 0m 37s the patch passed       Other Tests +1 unit 64m 39s hadoop-hdfs in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 89m 41s Subsystem Report/Notes Docker Image:yetus/hadoop:14b5c93 JIRA Issue HDFS-12217 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12879424/HDFS-12217.01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 070bf080bb55 3.13.0-123-generic #172-Ubuntu SMP Mon Jun 26 18:04:35 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 746189a Default Java 1.8.0_131 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-HDFS-Build/20477/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/20477/testReport/ modules C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs Console output https://builds.apache.org/job/PreCommit-HDFS-Build/20477/console Powered by Apache Yetus 0.6.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          manojg Manoj Govindassamy added a comment -

          Attached v01 patch to address the following
          1. LeaseManager#getINodesWithLease() verify if the file is not deleted before giving out the INodes.
          2. Added a new test in TestOpenFilesWithSnapshot to show how a pending create / open file deleted could cause problem when taking new snapshots. With the fix, the test passes through.
          Yongjun Zhang, can you please take a look at the patch ?

          Show
          manojg Manoj Govindassamy added a comment - Attached v01 patch to address the following 1. LeaseManager#getINodesWithLease() verify if the file is not deleted before giving out the INodes. 2. Added a new test in TestOpenFilesWithSnapshot to show how a pending create / open file deleted could cause problem when taking new snapshots. With the fix, the test passes through. Yongjun Zhang , can you please take a look at the patch ?

            People

            • Assignee:
              manojg Manoj Govindassamy
              Reporter:
              manojg Manoj Govindassamy
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development