joseluisll commented on code in PR #8777:
URL: https://github.com/apache/hadoop/pull/8777#discussion_r4211838088
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java:
##########
@@ -3091,6 +3091,22 @@ static ReplicaRecoveryInfo
initReplicaRecoveryImpl(String bpid, ReplicaMap map,
+ replica);
}
+ // A packet write that fails after its data reached the block file but
+ // before bytesOnDisk was updated (e.g. ClosedByInterruptException while
+ // syncing) leaves an unacknowledged tail in the block file. Drop it, as
+ // recoverRbwImpl does for pipeline recovery (HDFS-11472), instead of
+ // failing checkReplicaFiles and excluding the replica from recovery.
Review Comment:
Nit: `recoverRbwImpl` doesn't do this. It first raises `bytesOnDisk` to
`blockDataLength` and then truncates to `bytesAcked`. Truncating to
`bytesOnDisk` is the more conservative choice here, since the tail's checksum
may not have reached the meta file. Maybe say that instead.
##########
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/fsdataset/impl/FsDatasetImpl.java:
##########
@@ -3091,6 +3091,22 @@ static ReplicaRecoveryInfo
initReplicaRecoveryImpl(String bpid, ReplicaMap map,
+ replica);
}
+ // A packet write that fails after its data reached the block file but
+ // before bytesOnDisk was updated (e.g. ClosedByInterruptException while
+ // syncing) leaves an unacknowledged tail in the block file. Drop it, as
+ // recoverRbwImpl does for pipeline recovery (HDFS-11472), instead of
+ // failing checkReplicaFiles and excluding the replica from recovery.
+ final long bytesOnDisk = replica.getBytesOnDisk();
+ final long blockDataLength = replica.getBlockDataLength();
+ if (blockDataLength > bytesOnDisk) {
Review Comment:
Nit: this also runs for TEMPORARY replicas, where `getVisibleLength()`
returns -1, so the `bytesOnDisk >= visibleLength` guard above doesn't protect
anything for them. They're ignored by `BlockRecoveryWorker` anyway, so
restricting this to `replica.getState() == ReplicaState.RBW` would keep the
safety argument exact at no cost.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
