Copilot commented on code in PR #10105:
URL: https://github.com/apache/ozone/pull/10105#discussion_r3152167156
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -651,14 +651,14 @@ private void updateMetaData(KeyValueContainerData
containerData,
if (VersionedDatanodeFeatures.isFinalized(
HDDSLayoutFeature.STORAGE_SPACE_DISTRIBUTION) &&
delTX.hasTotalBlockSize()) {
long pendingBytes = containerData.getBlockPendingDeletionBytes();
- pendingBytes += delTX.getTotalBlockSize();
+ pendingBytes += delTX.getTotalSizePerReplica();
metadataTable
Review Comment:
`updateMetaData` gates pending-bytes updates on `delTX.hasTotalBlockSize()`
but then adds `delTX.getTotalSizePerReplica()`. If SCM/OM is older (no
`totalSizePerReplica` set) this will silently add 0 and undercount pending
deletion bytes. Consider checking `delTX.hasTotalSizePerReplica()` and falling
back to `delTX.getTotalBlockSize()` when the new field is absent.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -651,14 +651,14 @@ private void updateMetaData(KeyValueContainerData
containerData,
if (VersionedDatanodeFeatures.isFinalized(
HDDSLayoutFeature.STORAGE_SPACE_DISTRIBUTION) &&
delTX.hasTotalBlockSize()) {
long pendingBytes = containerData.getBlockPendingDeletionBytes();
- pendingBytes += delTX.getTotalBlockSize();
+ pendingBytes += delTX.getTotalSizePerReplica();
metadataTable
.putWithBatch(batchOperation,
containerData.getPendingDeleteBlockBytesKey(),
pendingBytes);
}
containerData.incrPendingDeletionBlocks(newDeletionBlocks,
- delTX.hasTotalBlockSize() ? delTX.getTotalBlockSize() : 0);
+ delTX.hasTotalBlockSize() ? delTX.getTotalSizePerReplica() : 0);
containerData.updateDeleteTransactionId(delTX.getTxID());
Review Comment:
`containerData.incrPendingDeletionBlocks(..., delTX.hasTotalBlockSize() ?
delTX.getTotalSizePerReplica() : 0)` has the same backward-compat issue: it
uses the presence of `totalBlockSize` to decide to apply bytes, but then reads
`totalSizePerReplica` (which defaults to 0 when unset). Use
`hasTotalSizePerReplica()` (or a computed `bytesToAdd` with fallback to
`totalBlockSize`) to avoid losing pending-deletion accounting during
mixed-version upgrades.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerDatanodeHeartbeatProtocol.proto:
##########
@@ -377,6 +377,7 @@ message DeletedBlocksTransaction {
optional int32 count = 4 [deprecated=true];
optional uint64 totalBlockSize = 5;
optional uint64 totalBlockReplicatedSize = 6;
+ optional uint64 totalSizePerReplica = 7;
}
Review Comment:
PR description says the proto change adds `requiredNodes` to
`DeletedBlocksTransaction`, but the actual change here adds
`totalSizePerReplica` instead. Please align the PR description with the
implemented wire format (or vice-versa) so reviewers/operators know what is
being rolled out and what compatibility expectations are.
--
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]
