From 36d4a49a4dcd223afbca419514b0fd4259a1b57b Mon Sep 17 00:00:00 2001 From: navinko Date: Tue, 19 May 2026 19:11:57 +0530 Subject: [PATCH 1/2] HDDS-15288. Use SequenceIdType in StateManagerImpl. --- .../hdds/scm/ha/SequenceIdGenerator.java | 15 +-- .../hdds/scm/ha/TestSequenceIDGenerator.java | 91 +++++++++++++++++++ 2 files changed, 99 insertions(+), 7 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java index 928032642fd..c19b3bc78e5 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java @@ -222,7 +222,7 @@ default RequestType getType() { static final class StateManagerImpl implements StateManager { private Table sequenceIdTable; private final DBTransactionBuffer transactionBuffer; - private final Map sequenceIdToLastIdMap; + private final Map sequenceIdToLastIdMap; private StateManagerImpl(Table sequenceIdTable, DBTransactionBuffer trxBuffer) { @@ -235,10 +235,11 @@ private StateManagerImpl(Table sequenceIdTable, @Override public Boolean allocateBatch(String sequenceIdName, Long expectedLastId, Long newLastId) { - Long lastId = sequenceIdToLastIdMap.computeIfAbsent(sequenceIdName, + SequenceIdType idType = SequenceIdType.valueOf(sequenceIdName); + Long lastId = sequenceIdToLastIdMap.computeIfAbsent(idType, key -> { try { - Long idInDb = this.sequenceIdTable.get(key); + Long idInDb = this.sequenceIdTable.get(key.name()); return idInDb != null ? idInDb : INVALID_SEQUENCE_ID; } catch (IOException ioe) { throw new RuntimeException("Failed to get lastId from db", ioe); @@ -253,18 +254,18 @@ public Boolean allocateBatch(String sequenceIdName, try { transactionBuffer - .addToBuffer(sequenceIdTable, sequenceIdName, newLastId); + .addToBuffer(sequenceIdTable, idType.name(), newLastId); } catch (IOException ioe) { throw new RuntimeException("Failed to put lastId to Batch", ioe); } - sequenceIdToLastIdMap.put(sequenceIdName, newLastId); + sequenceIdToLastIdMap.put(idType, newLastId); return true; } @Override public Long getLastId(String sequenceIdName) { - return sequenceIdToLastIdMap.get(sequenceIdName); + return sequenceIdToLastIdMap.get(SequenceIdType.valueOf(sequenceIdName)); } @Override @@ -286,7 +287,7 @@ private void initialize() throws IOException { "sequenceIdName should not be null"); Objects.requireNonNull(lastId, "lastId should not be null"); - sequenceIdToLastIdMap.put(sequenceIdName, lastId); + sequenceIdToLastIdMap.put(SequenceIdType.valueOf(sequenceIdName), lastId); } } } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java index 3c3418c1114..755da3cd129 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java @@ -19,6 +19,8 @@ import static org.apache.hadoop.hdds.scm.ScmConfigKeys.OZONE_SCM_SEQUENCE_ID_BATCH_SIZE; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.anyString; @@ -34,6 +36,7 @@ import org.apache.hadoop.hdds.scm.metadata.SCMMetadataStoreImpl; import org.apache.hadoop.hdds.utils.db.Table; import org.apache.hadoop.ozone.container.common.SCMTestUtils; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -180,4 +183,92 @@ public StateManager createStateManager( } } } + + @Test + public void testAllocateBatchFromDBWhenMissingInMap() throws Exception { + OzoneConfiguration conf = SCMTestUtils.getConf(testDir); + SCMMetadataStore scmMetadataStore = new SCMMetadataStoreImpl(conf); + scmMetadataStore.start(conf); + SCMHAManager scmHAManager = SCMHAManagerStub.getInstance(true); + + // Create the StateManager directly using its Builder + SequenceIdGenerator.StateManager stateManager = + new SequenceIdGenerator.StateManagerImpl.Builder() + .setRatisServer(scmHAManager.getRatisServer()) + .setDBTransactionBuffer(scmHAManager.getDBTransactionBuffer()) + .setSequenceIdTable(scmMetadataStore.getSequenceIdTable()) + .build(); + + String localIdKey = SequenceIdType.localId.name(); + + // Verify initial state from empty DB + Assertions.assertNull(stateManager.getLastId(localIdKey)); + + // Allocate a new batch, which puts 100L into the sequenceIdToLastIdMap map + assertTrue(stateManager.allocateBatch(localIdKey, 0L, 100L)); + // Verify the map was updated + assertEquals(100L, stateManager.getLastId(localIdKey)); + + // Allocate a new batch, which puts 100L into the sequenceIdToLastIdMap map + assertTrue(stateManager.allocateBatch(localIdKey, 100L, 200L)); + // Verify the map was updated + assertEquals(200L, stateManager.getLastId(localIdKey)); + + // This call should fail because expectedLastId in db should be (200L) + // But we are passing 0L + assertFalse(stateManager.allocateBatch(localIdKey, 0L, 100L)); + } + + @Test + public void testReinitializePopulatesSequenceIdMapFromDB() throws Exception { + OzoneConfiguration conf = SCMTestUtils.getConf(testDir); + SCMMetadataStore scmMetadataStore = new SCMMetadataStoreImpl(conf); + scmMetadataStore.start(conf); + SCMHAManager scmHAManager = SCMHAManagerStub.getInstance(true); + + String containerId = SequenceIdType.containerId.name(); + // Simulate an SCM restart by writing a raw String directly to the database. + scmMetadataStore.getSequenceIdTable().put(containerId, 100L); + + // Create the StateManager directly using its Builder + SequenceIdGenerator.StateManager stateManager = + new SequenceIdGenerator.StateManagerImpl.Builder() + .setRatisServer(scmHAManager.getRatisServer()) + .setDBTransactionBuffer(scmHAManager.getDBTransactionBuffer()) + .setSequenceIdTable(scmMetadataStore.getSequenceIdTable()) + .build(); + + // Check if reinitialize() correctly converts DB key into SequenceIdType Enums + // for the sequenceIdToLastIdMap used. + stateManager.reinitialize(scmMetadataStore.getSequenceIdTable()); + + assertEquals(100L, stateManager.getLastId(containerId)); + assertTrue(stateManager.allocateBatch(containerId, 100L, 1100L)); + assertEquals(1100L, stateManager.getLastId(containerId)); + } + + @Test + public void testAllocateBatchFailsOnUnknownSequenceId() throws Exception { + OzoneConfiguration conf = SCMTestUtils.getConf(testDir); + SCMMetadataStore scmMetadataStore = new SCMMetadataStoreImpl(conf); + scmMetadataStore.start(conf); + SCMHAManager scmHAManager = SCMHAManagerStub.getInstance(true); + + // Create the StateManager directly using its Builder + SequenceIdGenerator.StateManager stateManager = + new SequenceIdGenerator.StateManagerImpl.Builder() + .setRatisServer(scmHAManager.getRatisServer()) + .setDBTransactionBuffer(scmHAManager.getDBTransactionBuffer()) + .setSequenceIdTable(scmMetadataStore.getSequenceIdTable()) + .build(); + + try { + // sequenceIdName string must match one of the predefined Enums. + // Passing an invalid string should immediately throw an exception before hitting the db. + stateManager.allocateBatch("unknownSequenceId", 0L, 1L); + fail("Expected allocateBatch to reject an unknown sequence id"); + } catch (Exception e) { + // ignore + } + } } From 38961e76fe4afa77d86aa7226516cfd0750057af Mon Sep 17 00:00:00 2001 From: navinko Date: Fri, 22 May 2026 02:04:35 +0530 Subject: [PATCH 2/2] HDDS-15288. Addressed review comments. --- .../hdds/scm/ha/SequenceIdGenerator.java | 10 ++++---- .../hdds/scm/ha/TestSequenceIDGenerator.java | 25 +++++++++---------- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java index c19b3bc78e5..415f0d14b12 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SequenceIdGenerator.java @@ -134,7 +134,7 @@ public long getNextId(SequenceIdType idType) throws SCMException { } // reload lastId from RocksDB. - batch.lastId = stateManager.getLastId(idType.name()); + batch.lastId = stateManager.getLastId(idType); } Preconditions.checkArgument(batch.nextId <= batch.lastId); @@ -198,10 +198,10 @@ Boolean allocateBatch(String sequenceIdName, throws SCMException; /** - * @param sequenceIdName : name of the sequence id. + * @param idType : supported sequence ID type. * @return lastId saved in db */ - Long getLastId(String sequenceIdName); + Long getLastId(SequenceIdType idType); /** * Reinitialize the SequenceIdGenerator with the latest sequenceIdTable @@ -264,8 +264,8 @@ public Boolean allocateBatch(String sequenceIdName, } @Override - public Long getLastId(String sequenceIdName) { - return sequenceIdToLastIdMap.get(SequenceIdType.valueOf(sequenceIdName)); + public Long getLastId(SequenceIdType idType) { + return sequenceIdToLastIdMap.get(idType); } @Override diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java index 755da3cd129..f7d8b1c499a 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSequenceIDGenerator.java @@ -199,24 +199,23 @@ public void testAllocateBatchFromDBWhenMissingInMap() throws Exception { .setSequenceIdTable(scmMetadataStore.getSequenceIdTable()) .build(); - String localIdKey = SequenceIdType.localId.name(); - + SequenceIdType idType = SequenceIdType.localId; // Verify initial state from empty DB - Assertions.assertNull(stateManager.getLastId(localIdKey)); + Assertions.assertNull(stateManager.getLastId(idType)); // Allocate a new batch, which puts 100L into the sequenceIdToLastIdMap map - assertTrue(stateManager.allocateBatch(localIdKey, 0L, 100L)); + assertTrue(stateManager.allocateBatch(idType.name(), 0L, 100L)); // Verify the map was updated - assertEquals(100L, stateManager.getLastId(localIdKey)); + assertEquals(100L, stateManager.getLastId(idType)); // Allocate a new batch, which puts 100L into the sequenceIdToLastIdMap map - assertTrue(stateManager.allocateBatch(localIdKey, 100L, 200L)); + assertTrue(stateManager.allocateBatch(idType.name(), 100L, 200L)); // Verify the map was updated - assertEquals(200L, stateManager.getLastId(localIdKey)); + assertEquals(200L, stateManager.getLastId(idType)); // This call should fail because expectedLastId in db should be (200L) // But we are passing 0L - assertFalse(stateManager.allocateBatch(localIdKey, 0L, 100L)); + assertFalse(stateManager.allocateBatch(idType.name(), 0L, 100L)); } @Test @@ -226,9 +225,9 @@ public void testReinitializePopulatesSequenceIdMapFromDB() throws Exception { scmMetadataStore.start(conf); SCMHAManager scmHAManager = SCMHAManagerStub.getInstance(true); - String containerId = SequenceIdType.containerId.name(); + SequenceIdType idType = SequenceIdType.containerId; // Simulate an SCM restart by writing a raw String directly to the database. - scmMetadataStore.getSequenceIdTable().put(containerId, 100L); + scmMetadataStore.getSequenceIdTable().put(idType.name(), 100L); // Create the StateManager directly using its Builder SequenceIdGenerator.StateManager stateManager = @@ -242,9 +241,9 @@ public void testReinitializePopulatesSequenceIdMapFromDB() throws Exception { // for the sequenceIdToLastIdMap used. stateManager.reinitialize(scmMetadataStore.getSequenceIdTable()); - assertEquals(100L, stateManager.getLastId(containerId)); - assertTrue(stateManager.allocateBatch(containerId, 100L, 1100L)); - assertEquals(1100L, stateManager.getLastId(containerId)); + assertEquals(100L, stateManager.getLastId(idType)); + assertTrue(stateManager.allocateBatch(idType.name(), 100L, 1100L)); + assertEquals(1100L, stateManager.getLastId(idType)); } @Test