HDFS-7434. DatanodeID hashCode should not be mutable. Contributed by Daryn Sharp.
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/e93eee9f Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/e93eee9f Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/e93eee9f Branch: refs/heads/HDFS-7285 Commit: e93eee9f69436174e83f194fc8d82c8a581ad1f2 Parents: ba4d888 Author: Kihwal Lee <kih...@apache.org> Authored: Wed Mar 4 17:21:51 2015 -0600 Committer: Jing Zhao <ji...@apache.org> Committed: Mon Mar 9 13:11:24 2015 -0700 ---------------------------------------------------------------------- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 2 + .../apache/hadoop/hdfs/protocol/DatanodeID.java | 48 ++++++++------------ .../server/protocol/DatanodeRegistration.java | 10 ++++ .../blockmanagement/TestBlockManager.java | 7 --- .../TestComputeInvalidateWork.java | 16 +++++-- .../TestDatanodeProtocolRetryPolicy.java | 3 +- 6 files changed, 43 insertions(+), 43 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/e93eee9f/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 3c6d447..2be1a4c 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -1091,6 +1091,8 @@ Release 2.7.0 - UNRELEASED HDFS-7879. hdfs.dll does not export functions of the public libhdfs API. (Chris Nauroth via wheat9) + HDFS-7434. DatanodeID hashCode should not be mutable. (daryn via kihwal) + BREAKDOWN OF HDFS-7584 SUBTASKS AND RELATED JIRAS HDFS-7720. Quota by Storage Type API, tools and ClientNameNode http://git-wip-us.apache.org/repos/asf/hadoop/blob/e93eee9f/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeID.java ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeID.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeID.java index 779e3b9..f91696f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeID.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/protocol/DatanodeID.java @@ -47,19 +47,23 @@ public class DatanodeID implements Comparable<DatanodeID> { private int infoSecurePort; // info server port private int ipcPort; // IPC server port private String xferAddr; - private int hashCode = -1; /** * UUID identifying a given datanode. For upgraded Datanodes this is the * same as the StorageID that was previously used by this Datanode. * For newly formatted Datanodes it is a UUID. */ - private String datanodeUuid = null; + private final String datanodeUuid; public DatanodeID(DatanodeID from) { + this(from.getDatanodeUuid(), from); + } + + @VisibleForTesting + public DatanodeID(String datanodeUuid, DatanodeID from) { this(from.getIpAddr(), from.getHostName(), - from.getDatanodeUuid(), + datanodeUuid, from.getXferPort(), from.getInfoPort(), from.getInfoSecurePort(), @@ -81,19 +85,24 @@ public class DatanodeID implements Comparable<DatanodeID> { */ public DatanodeID(String ipAddr, String hostName, String datanodeUuid, int xferPort, int infoPort, int infoSecurePort, int ipcPort) { - this.ipAddr = ipAddr; + setIpAndXferPort(ipAddr, xferPort); this.hostName = hostName; this.datanodeUuid = checkDatanodeUuid(datanodeUuid); - this.xferPort = xferPort; this.infoPort = infoPort; this.infoSecurePort = infoSecurePort; this.ipcPort = ipcPort; - updateXferAddrAndInvalidateHashCode(); } public void setIpAddr(String ipAddr) { + //updated during registration, preserve former xferPort + setIpAndXferPort(ipAddr, xferPort); + } + + private void setIpAndXferPort(String ipAddr, int xferPort) { + // build xferAddr string to reduce cost of frequent use this.ipAddr = ipAddr; - updateXferAddrAndInvalidateHashCode(); + this.xferPort = xferPort; + this.xferAddr = ipAddr + ":" + xferPort; } public void setPeerHostName(String peerHostName) { @@ -107,12 +116,6 @@ public class DatanodeID implements Comparable<DatanodeID> { return datanodeUuid; } - @VisibleForTesting - public void setDatanodeUuidForTesting(String datanodeUuid) { - this.datanodeUuid = datanodeUuid; - updateXferAddrAndInvalidateHashCode(); - } - private String checkDatanodeUuid(String uuid) { if (uuid == null || uuid.isEmpty()) { return null; @@ -242,11 +245,7 @@ public class DatanodeID implements Comparable<DatanodeID> { @Override public int hashCode() { - if (hashCode == -1) { - int newHashCode = xferAddr.hashCode() ^ datanodeUuid.hashCode(); - hashCode = newHashCode & Integer.MAX_VALUE; - } - return hashCode; + return datanodeUuid.hashCode(); } @Override @@ -259,14 +258,12 @@ public class DatanodeID implements Comparable<DatanodeID> { * Note that this does not update storageID. */ public void updateRegInfo(DatanodeID nodeReg) { - ipAddr = nodeReg.getIpAddr(); + setIpAndXferPort(nodeReg.getIpAddr(), nodeReg.getXferPort()); hostName = nodeReg.getHostName(); peerHostName = nodeReg.getPeerHostName(); - xferPort = nodeReg.getXferPort(); infoPort = nodeReg.getInfoPort(); infoSecurePort = nodeReg.getInfoSecurePort(); ipcPort = nodeReg.getIpcPort(); - updateXferAddrAndInvalidateHashCode(); } /** @@ -279,13 +276,4 @@ public class DatanodeID implements Comparable<DatanodeID> { public int compareTo(DatanodeID that) { return getXferAddr().compareTo(that.getXferAddr()); } - - // NOTE: mutable hash codes are dangerous, however this class chooses to - // use them. this method must be called when a value mutates that is used - // to compute the hash, equality, or comparison of instances. - private void updateXferAddrAndInvalidateHashCode() { - xferAddr = ipAddr + ":" + xferPort; - // can't compute new hash yet because uuid might still null... - hashCode = -1; - } } http://git-wip-us.apache.org/repos/asf/hadoop/blob/e93eee9f/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeRegistration.java ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeRegistration.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeRegistration.java index aaa18c6..9db2fca 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeRegistration.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeRegistration.java @@ -25,6 +25,8 @@ import org.apache.hadoop.hdfs.security.token.block.ExportedBlockKeys; import org.apache.hadoop.hdfs.server.common.Storage; import org.apache.hadoop.hdfs.server.common.StorageInfo; +import com.google.common.annotations.VisibleForTesting; + /** * DatanodeRegistration class contains all information the name-node needs * to identify and verify a data-node when it contacts the name-node. @@ -39,6 +41,14 @@ public class DatanodeRegistration extends DatanodeID private ExportedBlockKeys exportedKeys; private final String softwareVersion; + @VisibleForTesting + public DatanodeRegistration(String uuid, DatanodeRegistration dnr) { + this(new DatanodeID(uuid, dnr), + dnr.getStorageInfo(), + dnr.getExportedKeys(), + dnr.getSoftwareVersion()); + } + public DatanodeRegistration(DatanodeID dn, StorageInfo info, ExportedBlockKeys keys, String softwareVersion) { super(dn); http://git-wip-us.apache.org/repos/asf/hadoop/blob/e93eee9f/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java index 236a583..97c9801 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java @@ -538,10 +538,6 @@ public class TestBlockManager { public void testSafeModeIBR() throws Exception { DatanodeDescriptor node = spy(nodes.get(0)); DatanodeStorageInfo ds = node.getStorageInfos()[0]; - - // TODO: Needs to be fixed. DatanodeUuid is not storageID. - node.setDatanodeUuidForTesting(ds.getStorageID()); - node.isAlive = true; DatanodeRegistration nodeReg = @@ -587,9 +583,6 @@ public class TestBlockManager { DatanodeDescriptor node = spy(nodes.get(0)); DatanodeStorageInfo ds = node.getStorageInfos()[0]; - // TODO: Needs to be fixed. DatanodeUuid is not storageID. - node.setDatanodeUuidForTesting(ds.getStorageID()); - node.isAlive = true; DatanodeRegistration nodeReg = http://git-wip-us.apache.org/repos/asf/hadoop/blob/e93eee9f/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java index fecca4e..5b08f53 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestComputeInvalidateWork.java @@ -38,7 +38,6 @@ import org.apache.hadoop.hdfs.server.namenode.FSNamesystem; import org.apache.hadoop.hdfs.server.protocol.DatanodeRegistration; import org.apache.hadoop.util.VersionInfo; import org.junit.After; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.mockito.internal.util.reflection.Whitebox; @@ -119,10 +118,17 @@ public class TestComputeInvalidateWork { public void testDatanodeReformat() throws Exception { namesystem.writeLock(); try { + // Change the datanode UUID to emulate a reformat + String poolId = cluster.getNamesystem().getBlockPoolId(); + DatanodeRegistration dnr = cluster.getDataNode(nodes[0].getIpcPort()) + .getDNRegistrationForBP(poolId); + dnr = new DatanodeRegistration(UUID.randomUUID().toString(), dnr); + cluster.stopDataNode(nodes[0].getXferAddr()); + Block block = new Block(0, 0, GenerationStamp.LAST_RESERVED_STAMP); bm.addToInvalidates(block, nodes[0]); - // Change the datanode UUID to emulate a reformation - nodes[0].setDatanodeUuidForTesting("fortesting"); + bm.getDatanodeManager().registerDatanode(dnr); + // Since UUID has changed, the invalidation work should be skipped assertEquals(0, bm.computeInvalidateWork(1)); assertEquals(0, bm.getPendingDeletionBlocksCount()); @@ -158,8 +164,8 @@ public class TestComputeInvalidateWork { // Re-register each DN and see that it wipes the invalidation work for (DataNode dn : cluster.getDataNodes()) { DatanodeID did = dn.getDatanodeId(); - did.setDatanodeUuidForTesting(UUID.randomUUID().toString()); - DatanodeRegistration reg = new DatanodeRegistration(did, + DatanodeRegistration reg = new DatanodeRegistration( + new DatanodeID(UUID.randomUUID().toString(), did), new StorageInfo(HdfsServerConstants.NodeType.DATA_NODE), new ExportedBlockKeys(), VersionInfo.getVersion()); http://git-wip-us.apache.org/repos/asf/hadoop/blob/e93eee9f/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDatanodeProtocolRetryPolicy.java ---------------------------------------------------------------------- diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDatanodeProtocolRetryPolicy.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDatanodeProtocolRetryPolicy.java index da858cd..ac7ebc0 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDatanodeProtocolRetryPolicy.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestDatanodeProtocolRetryPolicy.java @@ -173,7 +173,8 @@ public class TestDatanodeProtocolRetryPolicy { } else { DatanodeRegistration dr = (DatanodeRegistration) invocation.getArguments()[0]; - datanodeRegistration.setDatanodeUuidForTesting(dr.getDatanodeUuid()); + datanodeRegistration = + new DatanodeRegistration(dr.getDatanodeUuid(), dr); LOG.info("mockito succeeded " + datanodeRegistration); return datanodeRegistration; }
