This is a side-effect of HDFS-6423. Daryn Sharp has suggested that the quota check be done before converting inode/block. If something goes wrong, undoing the quota update is easier.

> ... . Daryn Sharp has suggested that the quota check be done before converting inode/block. ...

Sounds good. All the checks (quota, permission, etc.) should the performed before any change to the namespace.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12692046/HDFS-7587.patch
against trunk revision 10ac5ab.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9200//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9200//console

This message is automatically generated.

+    // MUST attempt quota update before changing in-memory states
+    updateQuotaForAppend(iip, file);
...
+    // may fail if block token creation fails, but we're still in a
+    // consistent state if the edit is logged first
+    return blockManager.convertLastBlockToUnderConstruction(file, 0);

I think we should use FSDirectory.verifyQuota(..) (instead updating quote) in the beginning and then update quota at the end. Otherwise, the quote counts will be incorrect if there is an exception thrown later on.

Agree with Tsz Wo Nicholas Sze we should use verifyQuota() instead of UpdateSpaceConsumed(). Also, can we add a unit test to verify that correctness of the quota usage after the exception is thrown for this case?

verifyQuota is already invoked so the quota counts shouldn't go out of sync. updateSpaceConsumed calls updateCount, which calls verifyQuota prior to invoking unprotectedUpdateCount. The quotas aren't going to change so it seems calling verifyQuota explicitly is wasted processing time.

Otherwise, the quote counts will be incorrect if there is an exception thrown later on.

Do you have a scenario in mind? Ie. what is "later on"? Moving the file to UC and associating the lease aren't going to throw checked exceptions. They might throw a runtime exception. The NN has no concept of a transaction (no rollback), so we're fully committed to finishing the op once we start updating datastructures. In this patch, once the quota update is successful, we're committed to moving the file to UC and assigning a lease. If we think those final steps will throw, then we're in trouble because we can't rollback. Even if that were to happen, an out of sync quota is better than a corrupted in-memory state and edit logs caused by the NN throwing runtime exceptions that don't cause an abort.

> ... The quotas aren't going to change so it seems calling verifyQuota explicitly is wasted processing time.

We may call verifyQuota in the beginning and update quota without checking at the end.

> Do you have a scenario in mind? Ie. what is "later on"? Moving the file to UC and associating the lease aren't going to throw checked exceptions. ...

convertLastBlockToUnderConstruction does throw IOException.

> ... Even if that were to happen, an out of sync quota is better than a corrupted in-memory state and edit logs caused by the NN throwing runtime exceptions that don't cause an abort.

Agree. Out of sync quota is better than a corrupted in-memory state. Also, in-sync quota is better than out of sync quota. We could have both in-sync quota and uncorrupted in-memory state. I think no one is saying that we prefer in-sync quota than uncorrupted in-memory state.

Hi Daryn Sharp, are you still working on this?

Kihwal Lee / Daryn Sharp / Tsz Wo Nicholas Sze, is this still a blocker for 2.7? Can progress be made in the next few days? I plan to cut an RC end of this week. Please update. Tx.

It is still valid. We will get to it today.

I am fine if we commit the current patch and then file a follow up JIRA for addressing this comment.

We have been running with this version of patch at scale for some time now. We will address any remaining concern in a separate jira.
+1.

The patch does not apply anymore.

The patch doesn't apply because the logic is very different due to truncate and variable length blocks.

At first glance, the new code looks buggy. It's sometimes billing quota, sometimes not, if the block exceeds the preferred size it appears you "earn" back quota. I don't have the familiarity with all this new code to provide a timely patch. Un-assigning myself. Jing Zhao, want to take a look?

Rebase Daryn's patch. Also make changes based on Nicholas's comments, i.e., first verifying the quota and updating the quota after the action.

With fix from HDFS-7943 we will not have blocks with size greater than the preferred block size. Thus we can avoid "earning back" quota scenarios.

Truncate may have similar issue when the data to truncate is only part of the original last block. Will update the patch later to fix this part.

Add fix for truncate.

Is it possible for the last block size to be greater than the preferred block size?

+      final long diff = (file.getPreferredBlockSize() - lastBlock.getNumBytes())
+          * file.getBlockReplication();

Thanks for the comment, Kihwal. Actually I mentioned this in my previous comment: with fix from HDFS-7943 we will not have blocks with size greater than the preferred block size. But I will appreciate if you can also confirm.

Sorry, I missed the comment. Should have refreshed.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12705151/HDFS-7587.001.patch
against trunk revision 32b4330.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.hdfs.server.namenode.ha.TestRetryCacheWithHA

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9942//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9942//console

This message is automatically generated.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12705176/HDFS-7587.002.patch
against trunk revision 968425e.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9946//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9946//console

This message is automatically generated.

• In verifyQuotaForUCBlock,
  • let's add a precondition check to make sure delta >= 0 (for append).
  • It should check if (!fsd.getFSNamesystem().isImageLoaded() || fsd.shouldSkipQuotaChecks()); see FSDirConcatOp.verifyQuota and FSDirRenameOp.verifyQuotaForRename.

• We should also verity quota by storage types; see FSDirConcatOp.computeQuotaDeltas.

• For truncate, the quota should be updated differently as following:
  • Copy-on-truncate for snapshot: need quota check for creating the new block. Quota usage count is increased.
  • Non-copy-on-truncate OR Copy-on-truncate for upgrade but not snapshot: Quota usage count is decreased. No quota check is needed.

Do you want to fix truncate in a different JIRA?

Thanks for the review, Nicholas! Update the patch to address your comments. I will separate the truncate fix into another jira.

Non-copy-on-truncate OR Copy-on-truncate for upgrade but not snapshot: Quota usage count is decreased. No quota check is needed.

We may also need to check/update the quota here since the current logic is to count UC block's storage usage using the preferred size.

+1 the new patch looks good.

> We may also need to check/update the quota here since the current logic is to count UC block's storage usage using the preferred size.

After truncate, there are two types for UC blocks, for creating/appending a block and for truncating a block. These quota counting for these two cases should be different. Let's discuss it in the truncate JIRA.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12705452/HDFS-7587.003.patch
against trunk revision c239b6d.

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/9966//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/9966//console

This message is automatically generated.

Thanks again for the review, Nicholas. I've committed this to 2.7.

FAILURE: Integrated in Hadoop-trunk-Commit #7368 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7368/)
HDFS-7587. Edit log corruption can happen if append fails with a quota violation. Contributed by Jing Zhao. (jing9: rev c7c71cdba50cb7d8282622cd496cc913c80cff54)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDiskspaceQuotaUpdate.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #137 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/137/)
HDFS-7587. Edit log corruption can happen if append fails with a quota violation. Contributed by Jing Zhao. (jing9: rev c7c71cdba50cb7d8282622cd496cc913c80cff54)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDiskspaceQuotaUpdate.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

FAILURE: Integrated in Hadoop-Yarn-trunk #871 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/871/)
HDFS-7587. Edit log corruption can happen if append fails with a quota violation. Contributed by Jing Zhao. (jing9: rev c7c71cdba50cb7d8282622cd496cc913c80cff54)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDiskspaceQuotaUpdate.java
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Hdfs-trunk #2069 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2069/)
HDFS-7587. Edit log corruption can happen if append fails with a quota violation. Contributed by Jing Zhao. (jing9: rev c7c71cdba50cb7d8282622cd496cc913c80cff54)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDiskspaceQuotaUpdate.java

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #128 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/128/)
HDFS-7587. Edit log corruption can happen if append fails with a quota violation. Contributed by Jing Zhao. (jing9: rev c7c71cdba50cb7d8282622cd496cc913c80cff54)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDiskspaceQuotaUpdate.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #137 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/137/)
HDFS-7587. Edit log corruption can happen if append fails with a quota violation. Contributed by Jing Zhao. (jing9: rev c7c71cdba50cb7d8282622cd496cc913c80cff54)

• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDiskspaceQuotaUpdate.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2087 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2087/)
HDFS-7587. Edit log corruption can happen if append fails with a quota violation. Contributed by Jing Zhao. (jing9: rev c7c71cdba50cb7d8282622cd496cc913c80cff54)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDiskspaceQuotaUpdate.java

For the 2.6.1 release effort, the backport isn't straightforward due to difference between 2.6 and 2.7. It has the following differences compared to the original patch.

• Include part of HDFS-7509 so that prepareFileForWrite has the expected function signature.
• Use Quota.Counts instead of QuotaCounts which is introduced in HDFS-7584.
• Skip the check for storage type specific quota introduced in HDFS-7584.
• Add the necessary definitions for INodesPath#length and FSDirectory#shouldSkipQuotaChecks.

Sangjin Lee backported this to 2.6.1. I just pushed the commit to 2.6.1 after running compilation and TestDiskspaceQuotaUpdate which changed in the patch.

Ming Ma, I didn't actually see a diff between the branch-2 patch and yours / Sangjin's. Appreciate any cross-verification on the 2.6.1 branch whether I got it right or not. Thanks.