joseluisll commented on code in PR #8777:
URL: https://github.com/apache/hadoop/pull/8777#discussion_r4211722405
##########
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:
Minor: the truncation runs before the generation-stamp and recovery-id
checks below. Harmless in practice, since a replica that fails them is stale
anyway, but moving it (together with `checkReplicaFiles`) next to the RUR
conversion would keep the on-disk change on the path that succeeds.
##########
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:
The comment says this mirrors `recoverRbwImpl`, but that method trusts the
file first (it raises `bytesOnDisk` to `blockDataLength`) and only then
truncates to `bytesAcked`. A DataNode restart also keeps this tail when its
checksums are valid, since the replica length is rebuilt by
`validateIntegrityAndSetLength`.
Truncating to `bytesOnDisk` seems like a fine conservative choice, since the
tail's checksums may never have reached the meta file. Could the comment say
that, instead of citing `recoverRbwImpl`?
--
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]
