smengcl commented on code in PR #11307:
URL: https://github.com/apache/ozone/pull/11307#discussion_r4300090874
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java:
##########
@@ -412,6 +419,198 @@ public void testCommitTransactions() throws Exception {
assertEquals(0, blocks.size());
}
+ // The tests below exercise SCM's purge decision in
SCMDeletedBlockTransactionStatusManager#commitTransactions.
+ // A replica added by a real container copy may already carry the
pending-delete record and self-heal on the
+ // datanode; these tests only verify that SCM keeps the transaction for
resend whenever it cannot rule out a
+ // replica that missed the deletion.
+ @Test
+ public void testCommitTransactionsDefersPurgeForReplicaAddedDuringCommit()
+ throws Exception {
+ deletedBlockLog.setScmCommandTimeoutMs(Long.MAX_VALUE);
+ addTransactions(generateData(1), true);
+ mockContainerHealthResult(true);
+ List<DeletedBlocksTransaction> blocks =
+ getTransactions(BLOCKS_PER_TXN * THREE);
+ assertEquals(THREE, blocks.size());
+ // All THREE entries above are the same logical transaction, one per
replica DN;
+ // commit it one DN at a time to control exactly how many times
+ // containerManager.getContainerReplicas() is invoked per commit call.
+ List<DeletedBlocksTransaction> singleTxn =
+ Collections.singletonList(blocks.get(0));
+
+ final ContainerID cid =
ContainerID.valueOf(blocks.get(0).getContainerID());
+ final Set<ContainerReplica> originalReplicas = new
HashSet<>(replicas.get(cid));
+
+ // Simulate ReplicationManager landing a new, never-acked replica of the
container
+ // in the window between the ack-set check and the durable purge: the
first three
+ // getContainerReplicas() calls (the in-loop decision for each of the
three commits
+ // below) see the original replica set, while every call after that (the
pre-purge
+ // recheck) sees a fourth replica that never received or acknowledged the
deletion.
+ ContainerReplica newReplica = ContainerReplica.newBuilder()
+ .setContainerID(cid)
+ .setContainerState(ContainerReplicaProto.State.CLOSED)
+
.setDatanodeDetails(DatanodeDetails.newBuilder().setUuid(UUID.randomUUID()).build())
+ .build();
+ Set<ContainerReplica> grownReplicas = new HashSet<>(originalReplicas);
+ grownReplicas.add(newReplica);
+ when(containerManager.getContainerReplicas(cid)).thenReturn(
+ originalReplicas, originalReplicas, originalReplicas, grownReplicas);
+
+ commitTransactions(singleTxn, dnList.get(0));
+ commitTransactions(singleTxn, dnList.get(1));
+ commitTransactions(singleTxn, dnList.get(2));
+
+ // The transaction must remain outstanding: the new replica never received
or
+ // acknowledged the delete, so purging now would leak its blocks forever.
+ assertEquals(1, deletedBlockLog.getNumOfValidTransactions(),
+ "transaction was purged although a never-acked replica exists");
+ assertEquals(1, deletedBlockLog.getTransactionToDNsCommitMapSize(),
+ "commit tracking was dropped although the transaction was not purged");
+ }
+
+ @Test
+ public void
testCommitTransactionsReadsEachContainerOnceAndDefersConsistently()
+ throws Exception {
+ deletedBlockLog.setScmCommandTimeoutMs(Long.MAX_VALUE);
+ mockContainerHealthResult(true);
+
+ // Two independent delete-block transactions against the SAME container (a
container
+ // commonly accumulates several separate pending deletions over time),
each already
+ // acknowledged by two of its three replicas before the decisive final ack
below.
+ Map<Long, List<DeletedBlock>> firstTxBlocks = generateData(1);
+ long containerID = firstTxBlocks.keySet().iterator().next();
+ List<DeletedBlock> secondTxBlocks = firstTxBlocks.get(containerID).stream()
+ .map(b -> new DeletedBlock(new BlockID(containerID,
b.getBlockID().getLocalID() + BLOCKS_PER_TXN),
+ b.getSize(), b.getReplicatedSize(), b.getSizePerReplica()))
+ .collect(Collectors.toList());
+ addTransactions(firstTxBlocks, true);
+ addTransactions(Collections.singletonMap(containerID, secondTxBlocks),
true);
+
+ List<DeletedBlocksTransaction> blocks = getTransactions(2 * BLOCKS_PER_TXN
* THREE);
+ assertEquals(2 * THREE, blocks.size());
+ Collection<DeletedBlocksTransaction> bothTxns = blocks.stream()
+ .collect(Collectors.toMap(DeletedBlocksTransaction::getTxID, t -> t,
(a, b) -> a))
+ .values();
+ assertEquals(2, bothTxns.size());
+
+ final ContainerID cid = ContainerID.valueOf(containerID);
+ final Set<ContainerReplica> originalReplicas = new
HashSet<>(replicas.get(cid));
+
+ commitTransactions(bothTxns, dnList.get(0));
+ commitTransactions(bothTxns, dnList.get(1));
+
+ // Simulate ReplicationManager landing a new, never-acked replica exactly
as the
+ // decisive final ack arrives: the ack-set check for each of the two txIDs
below sees
+ // the original replica set, while the pre-purge snapshot sees a fourth
replica that
+ // never received or acknowledged either deletion.
+ ContainerReplica newReplica = ContainerReplica.newBuilder()
+ .setContainerID(cid)
+ .setContainerState(ContainerReplicaProto.State.CLOSED)
+
.setDatanodeDetails(DatanodeDetails.newBuilder().setUuid(UUID.randomUUID()).build())
+ .build();
+ Set<ContainerReplica> grownReplicas = new HashSet<>(originalReplicas);
+ grownReplicas.add(newReplica);
+ when(containerManager.getContainerReplicas(cid)).thenReturn(
+ originalReplicas, originalReplicas, grownReplicas);
+ clearInvocations(containerManager);
+
+ commitTransactions(bothTxns, dnList.get(2));
+
+ // Two ack-set checks (one per txID) plus exactly one deduplicated
pre-purge snapshot
+ // shared by both txIDs of this container: three calls total, not four.
+ verify(containerManager, times(3)).getContainerReplicas(cid);
+
+ // Both transactions must stay outstanding and get the SAME verdict: a
never-acked
+ // replica now exists, so purging either would leak its blocks with no way
to resend.
+ assertEquals(2, deletedBlockLog.getNumOfValidTransactions(),
+ "both transactions for the container must stay outstanding once a
never-acked "
+ + "replica exists");
+ assertEquals(2, deletedBlockLog.getTransactionToDNsCommitMapSize(),
+ "commit tracking was dropped although neither transaction was purged");
+ }
+
+ @Test
+ public void
testCommitTransactionsDefersPurgeOnlyForContainerWithPendingAdd() throws
Exception {
+ deletedBlockLog.setScmCommandTimeoutMs(Long.MAX_VALUE);
+ mockContainerHealthResult(true);
+ addTransactions(generateData(2), true);
+ List<DeletedBlocksTransaction> blocks = getTransactions(2 * BLOCKS_PER_TXN
* THREE);
+ Collection<DeletedBlocksTransaction> bothTxns = blocks.stream()
+ .collect(Collectors.toMap(DeletedBlocksTransaction::getTxID, t -> t,
(a, b) -> a))
+ .values();
+ assertEquals(2, bothTxns.size());
+
+ // A copy of the first container is in flight; the other container has
none. The pending op does not
+ // depend on the order in which the candidates are checked.
+ final ContainerID pendingCid =
ContainerID.valueOf(bothTxns.iterator().next().getContainerID());
+ pendingOps.scheduleAddReplica(pendingCid,
DatanodeDetails.newBuilder().setUuid(UUID.randomUUID()).build(),
+ 0, mock(SCMCommand.class), Long.MAX_VALUE, 0L, 0L);
+
+ commitTransactions(bothTxns, dnList.get(0), dnList.get(1), dnList.get(2));
+
+ assertEquals(1, deletedBlockLog.getNumOfValidTransactions(),
+ "only the container without an in-flight replica ADD may be purged");
+ assertEquals(1, deletedBlockLog.getTransactionToDNsCommitMapSize());
+ }
+
+ @Test
+ public void testCommitTransactionsPurgesAfterCopiedReplicaAcknowledges()
throws Exception {
+ deletedBlockLog.setScmCommandTimeoutMs(Long.MAX_VALUE);
+ addTransactions(generateData(1), true);
+ mockContainerHealthResult(true);
+ List<DeletedBlocksTransaction> blocks = getTransactions(BLOCKS_PER_TXN *
THREE);
+ List<DeletedBlocksTransaction> singleTxn =
Collections.singletonList(blocks.get(0));
+ final ContainerID cid =
ContainerID.valueOf(blocks.get(0).getContainerID());
+
+ // A copy is scheduled (its export may predate the source recording the
deletion) and all original replicas
Review Comment:
This test only schedules a mocked ADD and mutates SCM's replica set. It does
not export a container before the source records the deletion, import that
copy, or process its container report.
Because pending-delete records are copied with the container DB, can we
cover that ordering explicitly? Without it, the test verifies transaction
retention but not the claimed permanent block leak.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java:
##########
@@ -619,6 +624,35 @@ public void
commitTransactions(List<DeleteBlockTransactionResult> transactionRes
transactionResult.getTxID(), e);
}
}
+ // A container copy (ReplicationManager) can add a replica after the
ack-set check above but before the
+ // durable removal below; purging then would leave that replica's blocks
undeleted with no way to resend.
+ // Each candidate container is therefore checked once: first for an
in-flight replica ADD, then against a
+ // fresh replica set. The order matters: ContainerStateManager registers a
replica and clears its pending
+ // ADD under the same container lock, so an ADD that is no longer pending
is already in the replica set.
+ final Map<ContainerID, List<DatanodeID>> replicaSnapshots = new
HashMap<>();
+ for (ContainerID containerId : new
HashSet<>(purgeCandidateContainers.values())) {
+ if (hasPendingAddReplica(containerId)) {
Review Comment:
The pending-operation check and replica snapshot are separate observations.
If the ADD expires or is scheduled after this check, but the datanode reports
the new replica after the snapshot and before removeTransactions(), this
transaction can still be purged before that replica acknowledges the deletion.
The new Javadoc also explicitly leaves this case uncovered below.
Can we coordinate purge with replica registration/finalization, or reconcile
delete progress when a late replica reports?
--
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]
