diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES_HDFS-2832.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES_HDFS-2832.txt index dd71b294b9e..227e8eb4c9e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES_HDFS-2832.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES_HDFS-2832.txt @@ -97,3 +97,5 @@ IMPROVEMENTS: HDFS-5491. Update editsStored for HDFS-2832. (Arpit Agarwal) + HDFS-5494. Fix findbugs warnings for HDFS-2832. (Arpit Agarwal) + diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPServiceActor.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPServiceActor.java index 5df81153ef8..a10768c3d27 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPServiceActor.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BPServiceActor.java @@ -98,7 +98,7 @@ class BPServiceActor implements Runnable { * reported to the NN. Access should be synchronized on this object. */ private final Map - pendingIncrementalBRperStorage = Maps.newConcurrentMap(); + pendingIncrementalBRperStorage = Maps.newHashMap(); private volatile int pendingReceivedRequests = 0; private volatile boolean shouldServiceRun = true; @@ -266,43 +266,54 @@ void reportBadBlocks(ExtendedBlock block, } /** - * Report received blocks and delete hints to the Namenode + * Report received blocks and delete hints to the Namenode for each + * storage. + * * @throws IOException */ private void reportReceivedDeletedBlocks() throws IOException { - // For each storage, check if there are newly received blocks and if - // so then send an incremental report to the NameNode. - for (Map.Entry entry : - pendingIncrementalBRperStorage.entrySet()) { - final String storageUuid = entry.getKey(); - final PerStoragePendingIncrementalBR perStorageMap = entry.getValue(); - ReceivedDeletedBlockInfo[] receivedAndDeletedBlockArray = null; - // TODO: We can probably use finer-grained synchronization now. - synchronized (pendingIncrementalBRperStorage) { + + // Generate a list of the pending reports for each storage under the lock + Map blockArrays = Maps.newHashMap(); + synchronized (pendingIncrementalBRperStorage) { + for (Map.Entry entry : + pendingIncrementalBRperStorage.entrySet()) { + final String storageUuid = entry.getKey(); + final PerStoragePendingIncrementalBR perStorageMap = entry.getValue(); + ReceivedDeletedBlockInfo[] receivedAndDeletedBlockArray = null; + if (perStorageMap.getBlockInfoCount() > 0) { // Send newly-received and deleted blockids to namenode receivedAndDeletedBlockArray = perStorageMap.dequeueBlockInfos(); pendingReceivedRequests -= receivedAndDeletedBlockArray.length; + blockArrays.put(storageUuid, receivedAndDeletedBlockArray); } } + } - if (receivedAndDeletedBlockArray != null) { - StorageReceivedDeletedBlocks[] report = { new StorageReceivedDeletedBlocks( - storageUuid, receivedAndDeletedBlockArray) }; - boolean success = false; - try { - bpNamenode.blockReceivedAndDeleted(bpRegistration, bpos.getBlockPoolId(), - report); - success = true; - } finally { + // Send incremental block reports to the Namenode outside the lock + for (Map.Entry entry : + blockArrays.entrySet()) { + final String storageUuid = entry.getKey(); + final ReceivedDeletedBlockInfo[] rdbi = entry.getValue(); + + StorageReceivedDeletedBlocks[] report = { new StorageReceivedDeletedBlocks( + storageUuid, rdbi) }; + boolean success = false; + try { + bpNamenode.blockReceivedAndDeleted(bpRegistration, + bpos.getBlockPoolId(), report); + success = true; + } finally { + if (!success) { synchronized (pendingIncrementalBRperStorage) { - if (!success) { - // If we didn't succeed in sending the report, put all of the - // blocks back onto our queue, but only in the case where we - // didn't put something newer in the meantime. - perStorageMap.putMissingBlockInfos(receivedAndDeletedBlockArray); - pendingReceivedRequests += perStorageMap.getBlockInfoCount(); - } + // If we didn't succeed in sending the report, put all of the + // blocks back onto our queue, but only in the case where we + // didn't put something newer in the meantime. + PerStoragePendingIncrementalBR perStorageMap = + pendingIncrementalBRperStorage.get(storageUuid); + perStorageMap.putMissingBlockInfos(rdbi); + pendingReceivedRequests += perStorageMap.getBlockInfoCount(); } } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java index b391a78d4c9..aad857c0eaa 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java @@ -293,7 +293,7 @@ protected void setPropertiesFromFields(Properties props, if (LayoutVersion.supports(Feature.ADD_DATANODE_AND_STORAGE_UUIDS, layoutVersion) && datanodeUuid != null) { - props.setProperty("datanodeUuid", datanodeUuid); + props.setProperty("datanodeUuid", getDatanodeUuid()); } // Set NamespaceID in version before federation @@ -348,7 +348,7 @@ protected void setFieldsFromProperties(Properties props, StorageDirectory sd) } else if (getDatanodeUuid().compareTo(dnUuid) != 0) { throw new InconsistentFSStateException(sd.getRoot(), "Root " + sd.getRoot() + ": DatanodeUuid=" + dnUuid + - ", does not match " + datanodeUuid + " from other" + + ", does not match " + getDatanodeUuid() + " from other" + " StorageDirectory."); } }