diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java index acefb5f0b38e..29f937bc0e69 100644 --- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java +++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java @@ -77,11 +77,8 @@ public final class ContainerInfo implements Comparable { // field and hence maintain the original output. @JsonIgnore private final ContainerID containerID; - // Delete Transaction Id is updated when new transaction for a container - // is stored in SCM delete Table. - // TODO: Replication Manager should consider deleteTransactionId so that - // replica with higher deleteTransactionId is preferred over replica with - // lower deleteTransactionId. + // Deprecated SCM-side delete transaction ID retained for old persisted data, SCM no longer updates this field. + @Deprecated private long deleteTransactionId; // The sequenceId of a close container cannot change, and all the // container replica should have the same sequenceId. @@ -218,6 +215,13 @@ public void setNumberOfKeys(long value) { numberOfKeys = value; } + /** + * Legacy SCM-side delete transaction ID. SCM no longer updates this field. + * + * @deprecated SCM no longer updates this field. Use DN-side container data + * for delete transaction tracking. + */ + @Deprecated public long getDeleteTransactionId() { return deleteTransactionId; } @@ -226,10 +230,6 @@ public long getSequenceId() { return sequenceId; } - public void updateDeleteTransactionId(long transactionId) { - deleteTransactionId = max(transactionId, deleteTransactionId); - } - public void updateSequenceId(long sequenceID) { assert (isOpen() || state == HddsProtos.LifeCycleState.QUASI_CLOSED); sequenceId = max(sequenceID, sequenceId); @@ -464,6 +464,7 @@ public Builder setOwner(String containerOwner) { return this; } + @Deprecated public Builder setDeleteTransactionId(long deleteTransactionID) { this.deleteTransactionId = deleteTransactionID; return this; diff --git a/hadoop-hdds/interface-client/src/main/proto/hdds.proto b/hadoop-hdds/interface-client/src/main/proto/hdds.proto index 091999675197..d61726e9642d 100644 --- a/hadoop-hdds/interface-client/src/main/proto/hdds.proto +++ b/hadoop-hdds/interface-client/src/main/proto/hdds.proto @@ -267,7 +267,8 @@ message ContainerInfoProto { required uint64 numberOfKeys = 5; optional int64 stateEnterTime = 6; required string owner = 7; - optional int64 deleteTransactionId = 8; + // Legacy SCM-side delete transaction ID. SCM no longer updates this field. + optional int64 deleteTransactionId = 8 [deprecated = true]; optional int64 sequenceId = 9; optional ReplicationFactor replicationFactor = 10; required ReplicationType replicationType = 11; diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManagerImpl.java index 7f45f5cb2d19..781f559ae8c5 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManagerImpl.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManagerImpl.java @@ -21,15 +21,12 @@ import com.google.protobuf.ByteString; import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; -import java.util.Map; import java.util.NoSuchElementException; import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import org.apache.hadoop.hdds.protocol.proto.HddsProtos.DeletedBlocksTransactionSummary; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.DeletedBlocksTransaction; -import org.apache.hadoop.hdds.scm.container.ContainerID; import org.apache.hadoop.hdds.scm.container.ContainerManager; import org.apache.hadoop.hdds.scm.ha.SCMHADBTransactionBuffer; import org.apache.hadoop.hdds.scm.ha.SCMRatisServer; @@ -53,7 +50,6 @@ public class DeletedBlockLogStateManagerImpl private Table deletedTable; private Table statefulConfigTable; - private ContainerManager containerManager; private final SCMHADBTransactionBuffer transactionBuffer; private final Set deletingTxIDs; public static final String SERVICE_NAME = DeletedBlockLogStateManager.class.getSimpleName(); @@ -62,7 +58,6 @@ public DeletedBlockLogStateManagerImpl(Table del Table statefulServiceConfigTable, ContainerManager containerManager, SCMHADBTransactionBuffer txBuffer) { this.deletedTable = deletedTable; - this.containerManager = containerManager; this.transactionBuffer = txBuffer; this.deletingTxIDs = ConcurrentHashMap.newKeySet(); this.statefulConfigTable = statefulServiceConfigTable; @@ -146,17 +141,12 @@ public void removeFromDB() { @Override public void addTransactionsToDB(ArrayList txs, DeletedBlocksTransactionSummary summary) throws IOException { - Map containerIdToTxnIdMap = new HashMap<>(); for (DeletedBlocksTransaction tx : txs) { - long tid = tx.getTxID(); - containerIdToTxnIdMap.compute(ContainerID.valueOf(tx.getContainerID()), - (k, v) -> v != null && v > tid ? v : tid); transactionBuffer.addToBuffer(deletedTable, tx.getTxID(), tx); } if (summary != null) { transactionBuffer.addToBuffer(statefulConfigTable, SERVICE_NAME, summary.toByteString()); } - containerManager.updateDeleteTransactionId(containerIdToTxnIdMap); } @Override diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java index bc7f05c70ec9..afc0461c1f7a 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManager.java @@ -21,7 +21,6 @@ import java.io.IOException; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.Set; import org.apache.hadoop.hdds.client.ReplicationConfig; import org.apache.hadoop.hdds.protocol.proto.HddsProtos.ContainerInfoProto; @@ -205,16 +204,6 @@ void updateContainerReplica(ContainerID containerID, ContainerReplica replica) void removeContainerReplica(ContainerID containerID, ContainerReplica replica) throws ContainerNotFoundException, ContainerReplicaNotFoundException; - /** - * Update deleteTransactionId according to deleteTransactionMap. - * - * @param deleteTransactionMap Maps the containerId to latest delete - * transaction id for the container. - * @throws IOException - */ - void updateDeleteTransactionId(Map deleteTransactionMap) - throws IOException; - default ContainerInfo getMatchingContainer(long size, String owner, Pipeline pipeline) { return getMatchingContainer(size, owner, pipeline, Collections.emptySet()); diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java index 337f5b849ac3..7fa5a323652d 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerManagerImpl.java @@ -22,7 +22,6 @@ import java.io.IOException; import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.NavigableSet; import java.util.Random; import java.util.Set; @@ -257,7 +256,6 @@ private ContainerInfo allocateContainer(final Pipeline pipeline, .setStateEnterTime(Time.now()) .setOwner(owner) .setContainerID(containerID.getId()) - .setDeleteTransactionId(0) .setReplicationType(pipeline.getType()); if (pipeline.getReplicationConfig() instanceof ECReplicationConfig) { @@ -357,12 +355,6 @@ public void removeContainerReplica(final ContainerID cid, } } - @Override - public void updateDeleteTransactionId( - final Map deleteTransactionMap) throws IOException { - containerStateManager.updateDeleteTransactionId(deleteTransactionMap); - } - @Override public ContainerInfo getMatchingContainer(final long size, final String owner, final Pipeline pipeline, final Set excludedContainerIDs) { diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java index caf7d79105fc..14075141eb2b 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManager.java @@ -193,13 +193,6 @@ void updateContainerStateWithSequenceId(HddsProtos.ContainerID id, void transitionDeletingOrDeletedToTargetState(HddsProtos.ContainerID id, LifeCycleState targetState) throws IOException; - /** - * - */ - // Make this as @Replicate - void updateDeleteTransactionId(Map deleteTransactionMap) - throws IOException; - /** * */ @@ -237,4 +230,14 @@ default RequestType getType() { void updateContainerInfo(HddsProtos.ContainerInfoProto containerInfo) throws IOException; + /** + * Legacy no-op retained for SCM HA compatibility with old method names. + * SCM no longer maintains ContainerInfo deleteTransactionId. + * + * @deprecated SCM no longer updates ContainerInfo deleteTransactionId. + */ + @Deprecated + void updateDeleteTransactionId(Map deleteTransactionMap) + throws IOException; + } diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java index 03a55e275015..513c8ca8db3f 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerStateManagerImpl.java @@ -484,28 +484,6 @@ public void removeContainerReplica(final ContainerReplica replica) { } } - @Override - public void updateDeleteTransactionId( - final Map deleteTransactionMap) throws IOException { - - // TODO: Refactor this. Error handling is not done. - for (Map.Entry transaction : - deleteTransactionMap.entrySet()) { - ContainerID containerID = transaction.getKey(); - try (AutoCloseableLock ignored = writeLock(containerID)) { - final ContainerInfo info = containers.getContainerInfo( - transaction.getKey()); - if (info == null) { - LOG.warn("Cannot find container {}, transaction id is {}", - transaction.getKey(), transaction.getValue()); - continue; - } - info.updateDeleteTransactionId(transaction.getValue()); - transactionBuffer.addToBuffer(containerStore, info.containerID(), info); - } - } - } - @Override public ContainerInfo getMatchingContainer(final long size, String owner, PipelineID pipelineID, NavigableSet containerIDs) { @@ -607,6 +585,12 @@ public void updateContainerInfo(HddsProtos.ContainerInfoProto updatedInfoProto) } } + @Override + public void updateDeleteTransactionId(Map deleteTransactionMap) { + // Legacy SCM HA method retained as a no-op. SCM no longer maintains the + // ContainerInfo deleteTransactionId; DN-side container data tracks it. + } + private AutoCloseableLock readLock() { return AutoCloseableLock.acquire(lock.readLock()); } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java index 1fb0adf97da7..850979f8ef49 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java @@ -25,7 +25,6 @@ import static org.junit.jupiter.params.provider.Arguments.arguments; import static org.mockito.Mockito.any; import static org.mockito.Mockito.atLeast; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -165,22 +164,6 @@ private void setupContainerManager() throws IOException { }); when(containerManager.getContainers()) .thenReturn(new ArrayList<>(containers.values())); - doAnswer(invocationOnMock -> { - Map map = - (Map) invocationOnMock.getArguments()[0]; - for (Map.Entry e : map.entrySet()) { - ContainerInfo info = containers.get(e.getKey()); - try { - assertThat(e.getValue()).isGreaterThan(info.getDeleteTransactionId()); - } catch (AssertionError err) { - throw new Exception("New TxnId " + e.getValue() + " < " + info - .getDeleteTransactionId()); - } - info.updateDeleteTransactionId(e.getValue()); - scmHADBTransactionBuffer.addToBuffer(containerTable, e.getKey(), info); - } - return null; - }).when(containerManager).updateDeleteTransactionId(any()); } private void updateContainerMetadata(long cid, @@ -312,7 +295,8 @@ private List getTransactions( } @Test - public void testContainerManagerTransactionId() throws Exception { + public void testAddTransactionsDoesNotUpdateContainerTransactionId() + throws Exception { // Initially all containers should have deleteTransactionId as 0 for (ContainerInfo containerInfo : containerManager.getContainers()) { assertEquals(0, containerInfo.getDeleteTransactionId()); @@ -329,11 +313,12 @@ public void testContainerManagerTransactionId() throws Exception { scmHADBTransactionBuffer.flush(); // After flush there should be 30 transactions in deleteTable - // All containers should have positive deleteTransactionId + // SCM does not update ContainerInfo deleteTransactionId when adding delete + // transactions. mockContainerHealthResult(true); assertEquals(30 * THREE, getAllTransactions().size()); for (ContainerInfo containerInfo : containerManager.getContainers()) { - assertThat(containerInfo.getDeleteTransactionId()).isGreaterThan(0); + assertEquals(0, containerInfo.getDeleteTransactionId()); } } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/invoker/ScmInvokerCodeGenerator.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/invoker/ScmInvokerCodeGenerator.java index 4ecc6263f780..4fc926d7fdee 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/invoker/ScmInvokerCodeGenerator.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/invoker/ScmInvokerCodeGenerator.java @@ -584,7 +584,7 @@ void printProxyClassMethod(Method method) { void printProxyClass() { printf("return new %s() {", apiName); try (UncheckedAutoCloseable ignored = printScope(false, 1)) { - for (Method m : getMethods(null, false)) { + for (Method m : getMethods(m -> m.getAnnotation(Deprecated.class) == null || !m.isDefault())) { printProxyClassMethod(m); } } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestBlockDeletion.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestBlockDeletion.java index 9b755f7e34bd..3186e35c9401 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestBlockDeletion.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestBlockDeletion.java @@ -250,7 +250,7 @@ public void testBlockDeletion(ReplicationConfig repConfig) throws Exception { // Delete transactionIds for the containers should be 0. // NOTE: this test assumes that all the container is KetValueContainer. If // other container types is going to be added, this test should be checked. - matchContainerTransactionIds(); + verifyDeleteTransactionIds(); assertEquals(0L, metrics.getNumBlockDeletionTransactionCreated()); @@ -293,8 +293,9 @@ public void testBlockDeletion(ReplicationConfig repConfig) throws Exception { // Few containers with deleted blocks assertThat(containerIdsWithDeletedBlocks).isNotEmpty(); - // Containers in the DN and SCM should have same delete transactionIds - matchContainerTransactionIds(); + // DN-side delete transactionIds should advance after deletion. SCM-side + // ContainerInfo deleteTransactionId is not updated by DeletedBlockLog. + verifyDeleteTransactionIds(); // Verify transactions committed GenericTestUtils.waitFor(() -> { @@ -308,11 +309,10 @@ public void testBlockDeletion(ReplicationConfig repConfig) throws Exception { } }, 500, 10000); - // After DN restart, containers in the DN and SCM should have same delete - // transactionIds. The assertion verifies that the state of containerInfos - // in DN and SCM is consistent after DN restart. + // After DN restart, delete transactionIds should remain persisted on DN. + // SCM-side ContainerInfo deleteTransactionId should remain unchanged. cluster.restartHddsDatanode(0, true); - matchContainerTransactionIds(); + verifyDeleteTransactionIds(); assertEquals(metrics.getNumBlockDeletionTransactionCreated(), metrics.getNumBlockDeletionTransactionCompleted()); @@ -716,7 +716,7 @@ private void verifyTransactionsCommitted() throws IOException { } } - private void matchContainerTransactionIds() throws IOException { + private void verifyDeleteTransactionIds() throws IOException { for (HddsDatanodeService datanode : cluster.getHddsDatanodes()) { ContainerSet dnContainerSet = datanode.getDatanodeStateMachine().getContainer().getContainerSet(); @@ -724,19 +724,17 @@ private void matchContainerTransactionIds() throws IOException { dnContainerSet.listContainer(0, 10000, containerDataList); for (ContainerData containerData : containerDataList) { long containerId = containerData.getContainerID(); + long dnDeleteTransactionId = + ((KeyValueContainerData) dnContainerSet.getContainer(containerId) + .getContainerData()).getDeleteTransactionId(); + assertEquals(0, + scm.getContainerInfo(containerId).getDeleteTransactionId()); if (containerIdsWithDeletedBlocks.contains(containerId)) { - assertThat(scm.getContainerInfo(containerId).getDeleteTransactionId()) - .isGreaterThan(0); - maxTransactionId = max(maxTransactionId, - scm.getContainerInfo(containerId).getDeleteTransactionId()); + assertThat(dnDeleteTransactionId).isGreaterThan(0); + maxTransactionId = max(maxTransactionId, dnDeleteTransactionId); } else { - assertEquals( - scm.getContainerInfo(containerId).getDeleteTransactionId(), 0); + assertEquals(0, dnDeleteTransactionId); } - assertEquals( - ((KeyValueContainerData) dnContainerSet.getContainer(containerId) - .getContainerData()).getDeleteTransactionId(), - scm.getContainerInfo(containerId).getDeleteTransactionId()); } } }