aajisaka commented on a change in pull request #3065:
URL: https://github.com/apache/hadoop/pull/3065#discussion_r647294150
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/ReplicaMap.java
##########
@@ -238,9 +221,10 @@ ReplicaInfo remove(String bpid, long blockId) {
* @return the number of replicas in the map
*/
int size(String bpid) {
+ LightWeightResizableGSet<Block, ReplicaInfo> m = null;
try (AutoCloseableLock l = readLock.acquire()) {
- FoldedTreeSet<ReplicaInfo> set = map.get(bpid);
- return set != null ? set.size() : 0;
+ m = map.get(bpid);
Review comment:
The definition of `m` can be moved into the try clause.
```suggestion
GSet<Block, ReplicaInfo> m = map.get(bpid);
```
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeStorageInfo.java
##########
@@ -262,30 +257,61 @@ public AddBlockResult addBlock(BlockInfo b, Block
reportedBlock) {
}
}
+ // add to the head of the data-node list
b.addStorage(this, reportedBlock);
- blocks.add(b);
+ insertToList(b);
return result;
}
AddBlockResult addBlock(BlockInfo b) {
return addBlock(b, b);
}
- boolean removeBlock(BlockInfo b) {
- blocks.remove(b);
- return b.removeStorage(this);
+ public void insertToList(BlockInfo b) {
+ blockList = b.listInsert(blockList, this);
+ numBlocks++;
+ }
+ public boolean removeBlock(BlockInfo b) {
+ blockList = b.listRemove(blockList, this);
+ if (b.removeStorage(this)) {
+ numBlocks--;
+ return true;
+ } else {
+ return false;
+ }
}
int numBlocks() {
- return blocks.size();
+ return numBlocks;
}
-
+
+ Iterator<BlockInfo> getBlockIterator() {
+ return new BlockIterator(blockList);
+ }
+
/**
- * @return iterator to an unmodifiable set of blocks
- * related to this {@link DatanodeStorageInfo}
+ * Move block to the head of the list of blocks belonging to the data-node.
+ * @return the index of the head of the blockList
*/
- Iterator<BlockInfo> getBlockIterator() {
- return Collections.unmodifiableSet(blocks).iterator();
+ int moveBlockToHead(BlockInfo b, int curIndex, int headIndex) {
+ blockList = b.moveBlockToHead(blockList, this, curIndex, headIndex);
+ return curIndex;
+ }
+
+ int getHeadIndex(DatanodeStorageInfo storageInfo) {
+ if (blockList == null) {
+ return -1;
+ }
+ return blockList.findStorageInfo(storageInfo);
+ }
Review comment:
This function is unused. It can be removed.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockHasMultipleReplicasOnSameDN.java
##########
@@ -118,13 +116,12 @@ public void testBlockHasMultipleReplicasOnSameDN() throws
IOException {
StorageBlockReport reports[] =
new StorageBlockReport[cluster.getStoragesPerDatanode()];
- ArrayList<ReplicaInfo> blocks = new ArrayList<>();
+ ArrayList<Replica> blocks = new ArrayList<Replica>();
Review comment:
Nit:
```suggestion
ArrayList<Replica> blocks = new ArrayList<>();
```
##########
File path: hadoop-hdfs-project/hadoop-hdfs/src/main/proto/DatanodeProtocol.proto
##########
@@ -256,9 +256,6 @@ message BlockReportContextProto {
// The block report lease ID, or 0 if we are sending without a lease to
// bypass rate-limiting.
optional uint64 leaseId = 4 [ default = 0 ];
-
- // True if the reported blocks are sorted by increasing block IDs
- optional bool sorted = 5 [default = false];
Review comment:
We must document that field number 5 cannot be reused.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
##########
@@ -1996,7 +1996,12 @@ private void metaSave(PrintWriter out) {
LightWeightHashSet<Long> openFileIds = new LightWeightHashSet<>();
for (DatanodeDescriptor dataNode :
blockManager.getDatanodeManager().getDatanodes()) {
- for (long ucFileId : dataNode.getLeavingServiceStatus().getOpenFiles()) {
+ // Sort open files
+ LightWeightHashSet<Long> dnOpenFiles =
+ dataNode.getLeavingServiceStatus().getOpenFiles();
+ Long[] dnOpenFileIds = new Long[dnOpenFiles.size()];
+ Arrays.sort(dnOpenFiles.toArray(dnOpenFileIds));
+ for (Long ucFileId : dnOpenFileIds) {
Review comment:
I thought we have to sort inode ID throughout the DataNodes. However, It
can be addressed in a separate jira because it is not sorted throughout the
DataNodes even before applying the patch.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/DatanodeStorageInfo.java
##########
@@ -262,30 +257,61 @@ public AddBlockResult addBlock(BlockInfo b, Block
reportedBlock) {
}
}
+ // add to the head of the data-node list
b.addStorage(this, reportedBlock);
- blocks.add(b);
+ insertToList(b);
return result;
}
AddBlockResult addBlock(BlockInfo b) {
return addBlock(b, b);
}
- boolean removeBlock(BlockInfo b) {
- blocks.remove(b);
- return b.removeStorage(this);
+ public void insertToList(BlockInfo b) {
+ blockList = b.listInsert(blockList, this);
+ numBlocks++;
+ }
+ public boolean removeBlock(BlockInfo b) {
Review comment:
I think `DatanodeStorageInfo#removeBlock` and
`ProvidedStorageMap#removeBlock` can be package-private.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlockManager.java
##########
@@ -1695,10 +1630,9 @@ private void verifyPlacementPolicy(final MiniDFSCluster
cluster,
LocatedBlock lb = DFSTestUtil.getAllBlocks(dfs, file).get(0);
BlockInfo blockInfo =
blockManager.getStoredBlock(lb.getBlock().getLocalBlock());
- Iterator<DatanodeStorageInfo> itr = blockInfo.getStorageInfos();
LOG.info("Block " + blockInfo + " storages: ");
- while (itr.hasNext()) {
- DatanodeStorageInfo dn = itr.next();
+ for (int i = 0; i < 3; i++) {
+ DatanodeStorageInfo dn = blockInfo.getStorageInfo(i);
Review comment:
Is there any specific reason to use `#getStorageInfo(i)` instead of
`#getStorageInfos()`? You need to assert the size >= 3?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
