ZanderXu commented on code in PR #6559:
URL: https://github.com/apache/hadoop/pull/6559#discussion_r1493941213
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java:
##########
@@ -2400,37 +2407,42 @@ public void invalidate(String bpid, ReplicaInfo block) {
}
/**
* Invalidate a block which is not found on disk. We should remove it from
- * memory and notify namenode, but unnecessary to delete the actual on-disk
- * block file again.
+ * memory and notify namenode, will decide whether to delete the actual
on-disk block and meta
+ * file based on {@link
DFSConfigKeys#DFS_DATANODE_DELETE_CORRUPT_REPLICA_FROM_DISK_ENABLE}.
*
* @param bpid the block pool ID.
* @param block The block to be invalidated.
* @param checkFiles Whether to check data and meta files.
*/
- public void invalidateMissingBlock(String bpid, Block block, boolean
checkFiles) {
+ public void invalidateMissingBlock(String bpid, Block block, boolean
checkFiles)
+ throws IOException {
// The replica seems is on its volume map but not on disk.
// We can't confirm here is block file lost or disk failed.
- // If block lost:
- // deleted local block file is completely unnecessary
- // If disk failed:
- // deleted local block file here may lead to missing-block
- // when it with only 1 replication left now.
- // So remove if from volume map notify namenode is ok.
+ // If checkFiles as true will check block or meta file existence again.
+ // If deleteCorruptReplicaFromDisk as true will delete the actual on-disk
block and meta file,
+ // otherwise will remove it from volume map and notify namenode.
Review Comment:
```
If checkFiles is true, the existence of the block and metafile will be
checked again.
If deleteCorruptReplicaFromDisk is true, delete the existing block or
metafile directly, otherwise just remove them from the memory volumeMap.
```
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java:
##########
@@ -2400,37 +2407,42 @@ public void invalidate(String bpid, ReplicaInfo block) {
}
/**
* Invalidate a block which is not found on disk. We should remove it from
- * memory and notify namenode, but unnecessary to delete the actual on-disk
- * block file again.
+ * memory and notify namenode, will decide whether to delete the actual
on-disk block and meta
+ * file based on {@link
DFSConfigKeys#DFS_DATANODE_DELETE_CORRUPT_REPLICA_FROM_DISK_ENABLE}.
*
* @param bpid the block pool ID.
* @param block The block to be invalidated.
* @param checkFiles Whether to check data and meta files.
*/
- public void invalidateMissingBlock(String bpid, Block block, boolean
checkFiles) {
+ public void invalidateMissingBlock(String bpid, Block block, boolean
checkFiles)
+ throws IOException {
// The replica seems is on its volume map but not on disk.
// We can't confirm here is block file lost or disk failed.
- // If block lost:
- // deleted local block file is completely unnecessary
- // If disk failed:
- // deleted local block file here may lead to missing-block
- // when it with only 1 replication left now.
- // So remove if from volume map notify namenode is ok.
+ // If checkFiles as true will check block or meta file existence again.
+ // If deleteCorruptReplicaFromDisk as true will delete the actual on-disk
block and meta file,
+ // otherwise will remove it from volume map and notify namenode.
try (AutoCloseableLock lock = lockManager.writeLock(LockLevel.BLOCK_POOl,
bpid)) {
// Check if this block is on the volume map.
ReplicaInfo replica = volumeMap.get(bpid, block);
// Double-check block or meta file existence when checkFiles as true.
if (replica != null && (!checkFiles ||
(!replica.blockDataExists() || !replica.metadataExists()))) {
- volumeMap.remove(bpid, block);
- invalidate(bpid, replica);
+ if (deleteCorruptReplicaFromDisk) {
+ ExtendedBlock extendedBlock = new ExtendedBlock(bpid, block);
+ datanode
+ .notifyNamenodeDeletedBlock(extendedBlock,
replica.getStorageUuid());
+ invalidate(bpid, new Block[] {extendedBlock.getLocalBlock()});
+ } else {
+ volumeMap.remove(bpid, block);
Review Comment:
Please add some comments to describe the necessity of the `else` logic.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java:
##########
@@ -2400,37 +2407,42 @@ public void invalidate(String bpid, ReplicaInfo block) {
}
/**
* Invalidate a block which is not found on disk. We should remove it from
- * memory and notify namenode, but unnecessary to delete the actual on-disk
- * block file again.
+ * memory and notify namenode, will decide whether to delete the actual
on-disk block and meta
+ * file based on {@link
DFSConfigKeys#DFS_DATANODE_DELETE_CORRUPT_REPLICA_FROM_DISK_ENABLE}.
*
* @param bpid the block pool ID.
* @param block The block to be invalidated.
* @param checkFiles Whether to check data and meta files.
*/
- public void invalidateMissingBlock(String bpid, Block block, boolean
checkFiles) {
+ public void invalidateMissingBlock(String bpid, Block block, boolean
checkFiles)
+ throws IOException {
// The replica seems is on its volume map but not on disk.
// We can't confirm here is block file lost or disk failed.
- // If block lost:
- // deleted local block file is completely unnecessary
- // If disk failed:
- // deleted local block file here may lead to missing-block
- // when it with only 1 replication left now.
- // So remove if from volume map notify namenode is ok.
+ // If checkFiles as true will check block or meta file existence again.
+ // If deleteCorruptReplicaFromDisk as true will delete the actual on-disk
block and meta file,
+ // otherwise will remove it from volume map and notify namenode.
try (AutoCloseableLock lock = lockManager.writeLock(LockLevel.BLOCK_POOl,
bpid)) {
// Check if this block is on the volume map.
ReplicaInfo replica = volumeMap.get(bpid, block);
// Double-check block or meta file existence when checkFiles as true.
if (replica != null && (!checkFiles ||
(!replica.blockDataExists() || !replica.metadataExists()))) {
- volumeMap.remove(bpid, block);
- invalidate(bpid, replica);
+ if (deleteCorruptReplicaFromDisk) {
+ ExtendedBlock extendedBlock = new ExtendedBlock(bpid, block);
+ datanode
+ .notifyNamenodeDeletedBlock(extendedBlock,
replica.getStorageUuid());
+ invalidate(bpid, new Block[] {extendedBlock.getLocalBlock()});
Review Comment:
`invalidate` this block first, then `notifyNameNodeDeletedBlock`.
--
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
