Initial patch

-1 overall

This message was automatically generated.

Thanks SammiChen for the patch. Adding few comments on the patch:

1. Missing new EC opcodes in editsStored.xml.
2. Update javadocs for:

   FSNamesystem#addErasureCodingPolicies, FSNamesystem#removeErasureCodingPolicy, FSNamesystem#enableErasureCodingPolicy, FSNamesystem#disableErasureCodingPolicy
   
   FSDirErasureCodingOp#addErasureCodingPolicy, FSDirErasureCodingOp#removeErasureCodingPolicy, FSDirErasureCodingOp#enableErasureCodingPolicy, FSDirErasureCodingOp#disableErasureCodingPolicy functions.
   

3. I could see CacheEntry and CacheEntryWithPayload used in the patch. Is it intentionally used differently for different cases?
4. Can we move checkOperation() and checkNameNodeSafeMode() checks outside for loop. If it throws exception, then #addErasureCodingPolicies() operation won't be atomic anyway, right? Atomic op, I meant either API should consider all policies and shouldn't add policies partially.

   FSNamesystem#addErasureCodingPolicies()
             ......
         for (ErasureCodingPolicy policy : policies) {
           try {
             checkOperation(OperationCategory.WRITE);
             checkNameNodeSafeMode("Cannot add erasure coding policy");
             ErasureCodingPolicy newPolicy =
                 FSDirErasureCodingOp.addErasureCodingPolicy(this, policy,
                     logRetryCache);
   

5. Looks like test case failures are related, please take care.

Thanks Rakesh R for the quick review!

I could see CacheEntry and CacheEntryWithPayload used in the patch. Is it intentionally used differently for different cases?

My understanding is CacheEntryWithPayload is used for API which has return object. CacheEntry is used fro API without return value.

Can we move checkOperation() and checkNameNodeSafeMode() checks outside for loop. If it throws exception, then #addErasureCodingPolicies() operation won't be atomic anyway, right? Atomic op, I meant either API should consider all policies and shouldn't add policies partially.

It's a good suggestion to move checkOperation() and checkNameNodeSafeMode() checks outside for loop. For addErasureCodingPolicies API itself, atomic is not a goal.

All other comments will be addressed in next patch.

My understanding is CacheEntryWithPayload is used for API which has return object. CacheEntry is used fro API without return value.

OK, fine.

002.patch uploaded. Here is the change list,
1. fixed style issues
2. fixed and improved the failed unit tests
3. Rakesh's comments are addressed.

Hi Rakesh R, I'm not very sure about the "Update javadocs" comments, is that because new parameter "logRetryCache" is added for involved API?

I separately uploaded the patch and editsStored file. If you want to apply the patch locally, you can first apply the patch, then replace the file under "hadoop-hdfs-project/hadoop-hdfs/src/test/resources/editsStored" with the one in this JIRA.

-1 overall

This message was automatically generated.

Thanks SammiChen for working on this. Having looked into the work and it looks overall close.

1. Overall, similar to HDFS-7859, this exposes lots of not-so-relevant changes already made in the patch or we need to make, so please feel free to open new issues to hold such changes separately. The essential changes for this issue is to log add/remove/enable/disable erasure coding policy.

2. You local change in pom.xml and editsStored.xml.

3. You changes in DFSClient looks like some bug fix to existing codes.

4. Refactor: getEcPolicy => getErasureCodingPolicy; AddECPolicyResponse => AddErasureCodingPolicyResponse

5. Don't quite like the way to sort the map by creating a tree map. And also, could we improve ECSchema to ensure extraOptions is sorted already, so we don't need to consider doing it in every places? If you'd do this, please in separate issue.

+      // Sort extra options based on key
+      extraOptions = new TreeMap<String, String>(extraOptions);

6. Please use some non-meaningful options for the test purpose to avoid possible confusion, like "testOption1" or the like.

+    Map<String, String> extraOptions = new HashMap<String, String>();
+    extraOptions.put("padding", "0");
+    extraOptions.put("recycle", "true");

7. Please try to use the same order when dump fields of erasure coding policy.

+  public static void writeErasureCodingPolicy(DataOutputStream out,
+      ErasureCodingPolicy ecPolicy) throws IOException {
+    writeInt(ecPolicy.getCellSize(), out);
+    writeString(ecPolicy.getSchema().getCodecName(), out);
+    writeInt(ecPolicy.getNumDataUnits(), out);
+    writeInt(ecPolicy.getNumParityUnits(), out);
}

+      XMLUtils.addSaxString(contentHandler, "CODEC", ecPolicy.getCodecName());
+      XMLUtils.addSaxString(contentHandler, "CELLSIZE",
+          Integer.toString(ecPolicy.getCellSize()));
+      XMLUtils.addSaxString(contentHandler, "DATAUNITS",
+          Integer.toString(ecPolicy.getNumDataUnits()));
+      XMLUtils.addSaxString(contentHandler, "PARITYUNITS",
+          Integer.toString(ecPolicy.getNumParityUnits()));

8. Not sure why we need to catch and convert the exception here, but not in other places. Better to pass the e instead of its

when convert to new IOException.

+    case OP_ADD_ERASURE_CODING_POLICY:
+      AddErasureCodingPolicyOp addOp = (AddErasureCodingPolicyOp) op;
+      try {
+        fsNamesys.getErasureCodingPolicyManager().addPolicy(
+            addOp.getEcPolicy());
+      } catch (HadoopIllegalArgumentException e) {
+        throw new IOException("Add erasure coding policy failed for:" +
+            e.getMessage());
+      }

Thanks SammiChen for the continuous efforts.

I'm not very sure about the "Update javadocs" comments, is that because new parameter "logRetryCache" is added for involved API?

Yes, good to could update javadoc,
(1) with new param.

   * @param logRetryCache whether to record RPC ids in editlog for retry cache
   *                      rebuilding

(2) Secondly, since you are touching FSDirErasureCodingOp functions, could you please add javadoc for the APIs which doesn't have details presently.

FSDirErasureCodingOp#addErasureCodingPolicy, FSDirErasureCodingOp#removeErasureCodingPolicy, FSDirErasureCodingOp#enableErasureCodingPolicy, FSDirErasureCodingOp#disableErasureCodingPolicy functions.

new editsStored raw file

Thanks Kai Zheng & Rakesh R reviewing the patch!

Following comments will be added later as the follow-ons. All other comments are addressed in the 003.patch

3. You changes in DFSClient looks like some bug fix to existing codes.
4. Refactor: getEcPolicy => getErasureCodingPolicy; AddECPolicyResponse => AddErasureCodingPolicyResponse
5. Don't quite like the way to sort the map by creating a tree map. And also, could we improve ECSchema to ensure extraOptions is sorted already, so we don't need to consider doing it in every places? If you'd do this, please in separate issue.

The latest patch looks pretty good and close. Only a minor:
Is this intended?

-    assertTrue("Edits " + editsStored + " should have all op codes",
-        hasAllOpCodes(editsStored));
+//    assertTrue("Edits " + editsStored + " should have all op codes",
+//        hasAllOpCodes(editsStored));

+1 pending on the fix.

Lei (Eddy) Xu, would you also take a look at this? Thanks. If no few more comments I'll get this in tomorrow.

-1 overall

This message was automatically generated.

LGTM.

	//    assertTrue("Edits " + editsStored + " should have all op codes",
        //        hasAllOpCodes(editsStored));

Is this going to be uncommented after OEV being implemented?

+1 pending.

Thanks Lei (Eddy) Xu and Kai Zheng! OEV is already been implemented in this JIRA since several unit test will failure without it. Th code is commented out during debug. Forget to uncomment it . Will make it correct in next patch.

1. fix style issues
2. uncomment the piece of code

Another note is the uploaded "editsStored" file is also a part of the patch. It's a binary file. After apply the HDFS-12395.004.patch to the trunk, need to replace the "editsStored" file under hadoop/hadoop-hdfs-project/hadoop-hdfs/src/test/resources with the uploaded one.

Otherwise one unit test will failure, just as the build system reports

org.apache.hadoop.hdfs.tools.offlineEditsViewer.TestOfflineEditsViewer.testStored
Error Details

Reference XML edits and parsed to XML should be same
Stack Trace
Standard Output

Thanks Sammi for the update. The latest patch LGTM. +1 pending on the building.

-1 overall

This message was automatically generated.

Checked the building results and it looks good. The relevant failure TestOfflineEditsViewer.testStored will be fixed after the check in, as commented by Sammi above. The minor check style will be addressed before check in.

Will commit it shortly.

Committed to branch-3.0 and trunk. Thanks SammiChen for the contribution, Rakesh R and Lei (Eddy) Xu for the review!

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12879 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12879/)
HDFS-12395. Support erasure coding policy operations in namenode edit (kai.zheng: rev 08d996d3e9265efad737efad50cbc5b10a0202f8)

• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestFSEditLogLoader.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/DFSTestUtil.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/resources/editsStored
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOpCodes.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/site/markdown/HdfsEditsViewer.md
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirErasureCodingOp.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeRpcServer.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/resources/editsStored.xml
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/tools/offlineEditsViewer/TestOfflineEditsViewer.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSImageSerialization.java

Following two test fails after this commit.

TestNamenodeRetryCache.testRetryCacheRebuild
TestRetryCacheWithHA.testRetryCacheOnStandbyNN

Reference:

https://builds.apache.org/job/PreCommit-HDFS-Build/21189/testReport/

Trace
java.lang.AssertionError: Retry cache size is wrong expected:<26> but was:<34>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:743)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:555)
at org.apache.hadoop.hdfs.server.namenode.TestNamenodeRetryCache.testRetryCacheRebuild(TestNamenodeRetryCache.java:439)

Hi, Brahma Reddy Battula, thanks for the reminder. I will address these two failed cases in HDFS-12460.

Thanks Brahma for raising this. I just got HDFS-12460 in. SammiChen please help double check with the latest trunk, if the two failures are fixed or not. Thanks.

Thanks for taking care, could have fixed the testcases in seperate jira since HDFS-12460 states "Make addErasureCodingPolicy an idempotent operation". Anyway it's committed,should be fine I feel.

You need to increment the NN layout version.

Kihwal Lee EC added LV -64 when the branch was merged. Since we've been in alpha up to now, the thinking was that we can piggyback on the existing EC layout version since we don't need to support upgrade from alpha->beta.

since we don't need to support upgrade from alpha->beta

In that case, it should be fine. I was thinking it needs to be bumped up since we already had a release with EC.

Thanks Kihwal Lee for the reminder of taking care the NN layout version, I have the same opinion as Andrew Wang.

And Brahma Reddy Battula, thanks for your advice. I will issue separate JIRA next time in this case.