HDDS-15634. Avoid updating container delete transaction ID on SCM delete log append

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerInfo.java

@@ -77,11 +77,8 @@ public final class ContainerInfo implements Comparable<ContainerInfo> {
   // 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;

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;

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<Long, DeletedBlocksTransaction> deletedTable;
   private Table<String, ByteString> statefulConfigTable;
-  private ContainerManager containerManager;
   private final SCMHADBTransactionBuffer transactionBuffer;
   private final Set<Long> deletingTxIDs;
   public static final String SERVICE_NAME = DeletedBlockLogStateManager.class.getSimpleName();
@@ -62,7 +58,6 @@ public DeletedBlockLogStateManagerImpl(Table<Long, DeletedBlocksTransaction> del
              Table<String, ByteString> 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<DeletedBlocksTransaction> txs,
       DeletedBlocksTransactionSummary summary) throws IOException {
-    Map<ContainerID, Long> 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

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<ContainerID, Long> deleteTransactionMap)
-      throws IOException;
-
   default ContainerInfo getMatchingContainer(long size, String owner,
                                      Pipeline pipeline) {
     return getMatchingContainer(size, owner, pipeline, Collections.emptySet());

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<ContainerID, Long> deleteTransactionMap) throws IOException {
-    containerStateManager.updateDeleteTransactionId(deleteTransactionMap);
-  }
-
   @Override
   public ContainerInfo getMatchingContainer(final long size, final String owner,
       final Pipeline pipeline, final Set<ContainerID> excludedContainerIDs) {

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<ContainerID, Long> 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<ContainerID, Long> deleteTransactionMap)
+      throws IOException;
+
 }

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<ContainerID, Long> deleteTransactionMap) throws IOException {
-
-    // TODO: Refactor this. Error handling is not done.
-    for (Map.Entry<ContainerID, Long> 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<ContainerID> containerIDs) {
@@ -607,6 +585,12 @@ public void updateContainerInfo(HddsProtos.ContainerInfoProto updatedInfoProto)
     }
   }
 
+  @Override
+  public void updateDeleteTransactionId(Map<ContainerID, Long> 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());
   }

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<ContainerID, Long> map =
-          (Map<ContainerID, Long>) invocationOnMock.getArguments()[0];
-      for (Map.Entry<ContainerID, Long> 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<DeletedBlocksTransaction> 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());
     }
   }
 

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);
       }
     }

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,27 +716,25 @@ 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();
       List<ContainerData> containerDataList = new ArrayList<>();
       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());
       }
     }
   }