diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index ad5f222b89e..94c7fafa27c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -502,6 +502,8 @@ Release 2.0.3-alpha - Unreleased HDFS-4214. OfflineEditsViewer should print out the offset at which it encountered an error. (Colin Patrick McCabe via atm) + HDFS-4199. Provide test for HdfsVolumeId. (Ivan A. Veselovsky via atm) + OPTIMIZATIONS BUG FIXES diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/HdfsVolumeId.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/HdfsVolumeId.java index 8e328051f7e..aa6785037c1 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/HdfsVolumeId.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/HdfsVolumeId.java @@ -27,26 +27,38 @@ import org.apache.hadoop.classification.InterfaceStability; * HDFS-specific volume identifier which implements {@link VolumeId}. Can be * used to differentiate between the data directories on a single datanode. This * identifier is only unique on a per-datanode basis. + * + * Note that invalid IDs are represented by {@link VolumeId#INVALID_VOLUME_ID}. */ @InterfaceStability.Unstable @InterfaceAudience.Public public class HdfsVolumeId implements VolumeId { - + private final byte[] id; - private final boolean isValid; - public HdfsVolumeId(byte[] id, boolean isValid) { + public HdfsVolumeId(byte[] id) { + if (id == null) { + throw new NullPointerException("A valid Id can only be constructed " + + "with a non-null byte array."); + } this.id = id; - this.isValid = isValid; } @Override - public boolean isValid() { - return isValid; + public final boolean isValid() { + return true; } @Override public int compareTo(VolumeId arg0) { + if (arg0 == null) { + return 1; + } + if (!arg0.isValid()) { + // any valid ID is greater + // than any invalid ID: + return 1; + } return hashCode() - arg0.hashCode(); } @@ -63,8 +75,10 @@ public class HdfsVolumeId implements VolumeId { if (obj == this) { return true; } - HdfsVolumeId that = (HdfsVolumeId) obj; + // NB: if (!obj.isValid()) { return false; } check is not necessary + // because we have class identity checking above, and for this class + // isValid() is always true. return new EqualsBuilder().append(this.id, that.id).isEquals(); } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/VolumeId.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/VolumeId.java index f24ed66d009..b756241e976 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/VolumeId.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/fs/VolumeId.java @@ -28,6 +28,48 @@ import org.apache.hadoop.classification.InterfaceStability; @InterfaceAudience.Public public interface VolumeId extends Comparable { + /** + * Represents an invalid Volume ID (ID for unknown content). + */ + public static final VolumeId INVALID_VOLUME_ID = new VolumeId() { + + @Override + public int compareTo(VolumeId arg0) { + // This object is equal only to itself; + // It is greater than null, and + // is always less than any other VolumeId: + if (arg0 == null) { + return 1; + } + if (arg0 == this) { + return 0; + } else { + return -1; + } + } + + @Override + public boolean equals(Object obj) { + // this object is equal only to itself: + return (obj == this); + } + + @Override + public int hashCode() { + return Integer.MIN_VALUE; + } + + @Override + public boolean isValid() { + return false; + } + + @Override + public String toString() { + return "Invalid VolumeId"; + } + }; + /** * Indicates if the disk identifier is valid. Invalid identifiers indicate * that the block was not present, or the location could otherwise not be diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockStorageLocationUtil.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockStorageLocationUtil.java index de74e023400..934f8dfe516 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockStorageLocationUtil.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockStorageLocationUtil.java @@ -202,7 +202,7 @@ class BlockStorageLocationUtil { ArrayList l = new ArrayList(b.getLocations().length); // Start off all IDs as invalid, fill it in later with results from RPCs for (int i = 0; i < b.getLocations().length; i++) { - l.add(new HdfsVolumeId(null, false)); + l.add(VolumeId.INVALID_VOLUME_ID); } blockVolumeIds.put(b, l); } @@ -236,7 +236,7 @@ class BlockStorageLocationUtil { // Get the VolumeId by indexing into the list of VolumeIds // provided by the datanode byte[] volumeId = metaVolumeIds.get(volumeIndex); - HdfsVolumeId id = new HdfsVolumeId(volumeId, true); + HdfsVolumeId id = new HdfsVolumeId(volumeId); // Find out which index we are in the LocatedBlock's replicas LocatedBlock locBlock = extBlockToLocBlock.get(extBlock); DatanodeInfo[] dnInfos = locBlock.getLocations(); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestVolumeId.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestVolumeId.java new file mode 100644 index 00000000000..da6f192a757 --- /dev/null +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/fs/TestVolumeId.java @@ -0,0 +1,183 @@ +/** + * 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 + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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.fs; + +import org.junit.Test; +import static org.junit.Assert.*; + +public class TestVolumeId { + + @Test + public void testEquality() { + final VolumeId id1 = new HdfsVolumeId(new byte[] { (byte)0, (byte)0 }); + testEq(true, id1, id1); + + final VolumeId id2 = new HdfsVolumeId(new byte[] { (byte)0, (byte)1 }); + testEq(true, id2, id2); + testEq(false, id1, id2); + + final VolumeId id3 = new HdfsVolumeId(new byte[] { (byte)1, (byte)0 }); + testEq(true, id3, id3); + testEq(false, id1, id3); + + // same as 2, but "invalid": + final VolumeId id2copy1 = new HdfsVolumeId(new byte[] { (byte)0, (byte)1 }); + + testEq(true, id2, id2copy1); + + // same as 2copy1: + final VolumeId id2copy2 = new HdfsVolumeId(new byte[] { (byte)0, (byte)1 }); + + testEq(true, id2, id2copy2); + + testEqMany(true, new VolumeId[] { id2, id2copy1, id2copy2 }); + + testEqMany(false, new VolumeId[] { id1, id2, id3 }); + } + + @SuppressWarnings("unchecked") + private void testEq(final boolean eq, Comparable id1, Comparable id2) { + final int h1 = id1.hashCode(); + final int h2 = id2.hashCode(); + + // eq reflectivity: + assertTrue(id1.equals(id1)); + assertTrue(id2.equals(id2)); + assertEquals(0, id1.compareTo((T)id1)); + assertEquals(0, id2.compareTo((T)id2)); + + // eq symmetry: + assertEquals(eq, id1.equals(id2)); + assertEquals(eq, id2.equals(id1)); + + // null comparison: + assertFalse(id1.equals(null)); + assertFalse(id2.equals(null)); + + // compareTo: + assertEquals(eq, 0 == id1.compareTo((T)id2)); + assertEquals(eq, 0 == id2.compareTo((T)id1)); + // compareTo must be antisymmetric: + assertEquals(sign(id1.compareTo((T)id2)), -sign(id2.compareTo((T)id1))); + + // compare with null should never return 0 to be consistent with #equals(): + assertTrue(id1.compareTo(null) != 0); + assertTrue(id2.compareTo(null) != 0); + + // check that hash codes did not change: + assertEquals(h1, id1.hashCode()); + assertEquals(h2, id2.hashCode()); + if (eq) { + // in this case the hash codes must be the same: + assertEquals(h1, h2); + } + } + + private static int sign(int x) { + if (x == 0) { + return 0; + } else if (x > 0) { + return 1; + } else { + return -1; + } + } + + @SuppressWarnings("unchecked") + private void testEqMany(final boolean eq, Comparable... volumeIds) { + Comparable vidNext; + int sum = 0; + for (int i=0; i