Hexiaoqiao commented on code in PR #6653: URL: https://github.com/apache/hadoop/pull/6653#discussion_r1536998408
########## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java: ########## @@ -6,9 +6,9 @@ * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * + * <p> Review Comment: Suggest keep the original License format because it will impact RAT checks. ########## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java: ########## @@ -681,14 +703,46 @@ private static class RenameOperation { this.srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, srcChild); // get the counts before rename - oldSrcCounts.add(withCount.getReferredINode().computeQuotaUsage(bsps)); + oldSrcCountsInSnapshot.add(withCount.getReferredINode().computeQuotaUsage(bsps)); } else if (srcChildIsReference) { // srcChild is reference but srcChild is not in latest snapshot withCount = (INodeReference.WithCount) srcChild.asReference() .getReferredINode(); } else { withCount = null; } + // set quota for src and dst, ignore src is in Snapshot or is Reference Review Comment: Suggest the annotation start with uppercase and end with period. ########## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java: ########## @@ -681,14 +703,46 @@ private static class RenameOperation { this.srcIIP = INodesInPath.replace(srcIIP, srcIIP.length() - 1, srcChild); // get the counts before rename - oldSrcCounts.add(withCount.getReferredINode().computeQuotaUsage(bsps)); + oldSrcCountsInSnapshot.add(withCount.getReferredINode().computeQuotaUsage(bsps)); } else if (srcChildIsReference) { // srcChild is reference but srcChild is not in latest snapshot withCount = (INodeReference.WithCount) srcChild.asReference() .getReferredINode(); } else { withCount = null; } + // set quota for src and dst, ignore src is in Snapshot or is Reference + this.srcSubTreeCountOp = withCount == null ? + quotaPair.getLeft() : Optional.empty(); + this.dstSubTreeCountOp = quotaPair.getRight(); + } + + boolean isSameStoragePolicy() { Review Comment: Not sure to get what this method purpose. Another side, the first feeling is to calculate if `src` and `dst` has the same StoragePolicy, but it is not actually same implement as `function name`, right? ########## hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestCorrectnessOfQuotaAfterRenameOp.java: ########## @@ -0,0 +1,163 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> Review Comment: as above comment. ########## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirRenameOp.java: ########## @@ -638,16 +654,22 @@ private static class RenameOperation { private final byte[] srcChildName; private final boolean isSrcInSnapshot; private final boolean srcChildIsReference; - private final QuotaCounts oldSrcCounts; + private final QuotaCounts oldSrcCountsInSnapshot; + private final boolean sameStoragePolicy; + private final Optional<QuotaCounts> srcSubTreeCountOp; Review Comment: +1 ########## hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestCorrectnessOfQuotaAfterRenameOp.java: ########## @@ -0,0 +1,163 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode; + +import org.apache.hadoop.fs.ContentSummary; +import org.apache.hadoop.fs.Options; +import org.apache.hadoop.fs.Path; + +import org.apache.hadoop.hdfs.DFSTestUtil; +import org.apache.hadoop.hdfs.DistributedFileSystem; +import org.apache.hadoop.hdfs.HdfsConfiguration; +import org.apache.hadoop.hdfs.MiniDFSCluster; +import org.apache.hadoop.hdfs.protocol.HdfsConstants; +import org.apache.hadoop.hdfs.server.blockmanagement.BlockStoragePolicySuite; +import org.apache.hadoop.hdfs.server.namenode.snapshot.Snapshot; +import org.apache.hadoop.test.GenericTestUtils; +import org.apache.hadoop.test.PathUtils; +import org.junit.BeforeClass; +import org.junit.Test; + + Review Comment: Please remove redundant blank line. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
