HDDS-15145. Use enum for the ID type in SequenceIdGenerator by navinko · Pull Request #10211 · apache/ozone

navinko · 2026-05-07T18:34:03Z

What changes were proposed in this pull request?

HDDS-15145. Use enum for the ID type in SequenceIdGenerator

Please describe your PR in detail:

This PR introduces "SequenceIdType" to replace String based ID type arguments in "SequenceIdGenerator".
SequenceIdGenerator#getNextId now accepts "SequenceIdType" instead of String values, and its in-memory batch cache uses an EnumMap<SequenceIdType, Batch>.
This gives compile-time safety for supported sequence ID types while preserving the existing RocksDB key strings through "SequenceIdType#getDbKey()".
The existing String constants in SequenceIdGenerator are retained and now mapped with Enum .

localId, delTxnId, containerId, CertificateId, rootCertificateId
ROOT_CERTIFICATE_ID` remains deprecated, as it was already deprecated before this change.

[Reverted]Updated StateManager and StateManagerImpl to use Enum instead of String as sequenceID and created new codec for it. parameters for allocateBatch and getLastId are now updated to adapt Enum.
Added unit tests for SequenceIdType and updated existing SequenceIdGenerator and RootCARotationManager
tests
[Removed] Last Created new test for Codec
TestScmSequenceIdTypeCodec
LegacyStringSequenceIdCodecForTesting

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15145

How was this patch tested?

Validated with unit test and Also verified the change in a local Docker compose secure HA cluster. The SCM logs show enum-based allocation paths:

% docker compose logs -f scm1.org 2>&1 | grep -E "SequenceId|Allocate a batch|LOCAL_ID|CONTAINER_ID|DEL_TXN_ID|CERTIFICATE_ID"
scm1.org-1 | 2026-05-14 19:55:32,157 [main] INFO ha.SequenceIdGenerator: upgrade localId to 117883640217600000
scm1.org-1 | 2026-05-14 19:55:32,157 [main] INFO ha.SequenceIdGenerator: upgrade delTxnId to 0
scm1.org-1 | 2026-05-14 19:55:32,158 [main] INFO ha.SequenceIdGenerator: upgrade containerId to 0
scm1.org-1 | 2026-05-14 19:55:32,159 [main] INFO ha.SequenceIdGenerator: upgrade CertificateId to 2
scm1.org-1 | 2026-05-14 19:55:32,159 [main] INFO ha.SequenceIdGenerator: Init the HA SequenceIdGenerator.
scm1.org-1 | 2026-05-14 19:55:32,278 [main] INFO ha.SequenceIdGenerator: upgrade CertificateId to 2
scm1.org-1 | 2026-05-14 19:55:39,179 [7a2e5ec6-03ea-453a-a428-fdf6397aa440@group-6079C4D2F365-StateMachineUpdater] WARN ha.SequenceIdGenerator: Failed to allocate a batch for CertificateId, expected lastId is 0, actual lastId is 2.
scm1.org-1 | 2026-05-14 19:55:39,182 [IPC Server handler 0 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 2 to 3.
scm1.org-1 | 2026-05-14 19:55:41,929 [IPC Server handler 0 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 3 to 4.
scm1.org-1 | 2026-05-14 19:55:43,348 [IPC Server handler 1 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 4 to 5.
scm1.org-1 | 2026-05-14 19:55:49,346 [IPC Server handler 0 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 5 to 6.
scm1.org-1 | 2026-05-14 19:55:49,384 [IPC Server handler 1 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 6 to 7.
scm1.org-1 | 2026-05-14 19:55:49,412 [IPC Server handler 0 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 7 to 8.
scm1.org-1 | 2026-05-14 19:55:49,442 [IPC Server handler 1 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 8 to 9.
scm1.org-1 | 2026-05-14 19:55:49,473 [IPC Server handler 0 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 9 to 10.
scm1.org-1 | 2026-05-14 19:55:49,506 [IPC Server handler 1 on default port 9961] INFO ha.SequenceIdGenerator: Allocate a batch for CertificateId, change lastId from 10 to 11.
scm1.org-1 | 2026-05-14 20:01:59,944 [IPC Server handler 6 on default port 9863] INFO ha.SequenceIdGenerator: Allocate a batch for containerId, change lastId from 0 to 1000.
scm1.org-1 | 2026-05-14 20:01:59,957 [7a2e5ec6-03ea-453a-a428-fdf6397aa440@group-6079C4D2F365-StateMachineUpdater] WARN ha.SequenceIdGenerator: Failed to allocate a batch for localId, expected lastId is 0, actual lastId is 117883640217600000.
scm1.org-1 | 2026-05-14 20:01:59,961 [IPC Server handler 6 on default port 9863] INFO ha.SequenceIdGenerator: Allocate a batch for localId, change lastId from 117883640217600000 to 117883640217601000.
scm1.org-1 | 2026-05-14 20:05:53,603 [IPC Server handler 8 on default port 9863] INFO ha.SequenceIdGenerator: Allocate a batch for delTxnId, change lastId from 0 to 1000.

Latest progressing CI : https://github.com/navinko/ozone/actions/runs/25883196408

sreejasahithi

Thanks @navinko

sreejasahithi · 2026-05-08T06:40:11Z

+  public void testNumberOfEnumConstants() {
+    // If a new SequenceIdType is added, this test will fail.
+    // This serves as a reminder to the developer to verify RocksDB backward
+    // compatibility and update this test class accordingly.
+    assertEquals(5, SequenceIdType.values().length);
+  }


Here testNumberOfEnumConstants (assertEquals(5, SequenceIdType.values().length)) is fully subsumed by testIfNewEnumConstantGetsAdded. Every failure scenario that testNumberOfEnumConstants catches is also caught by testIfNewEnumConstantGetsAdded, which additionally catches renames (count stays 5 but a name changes), and provides a far more descriptive failure message with the "ACTION REQUIRED" instruction.

Thanks , it make sense!
removed this one.

sreejasahithi · 2026-05-08T07:33:17Z

+  @Test
+  public void testReturnsNullIfEnumConstantNotAvailable() {
+    assertNull(SequenceIdType.fromDbKey("unmapped-key-string"));
+    assertNull(SequenceIdType.fromDbKey(null));
+  }


We can in a different test consider asserting assertSame(type, fromDbKey(type.getDbKey())) for every constant so the static map stays aligned with getDbKey(), not covered by the unknown/null case alone.

Thanks @sreejasahithi for review, to align with current review comments i ahve removed the unused method fromDbKey completely.

szetszwo

@navinko , thanks for working on this! Please see the comments inlined.

szetszwo · 2026-05-08T18:26:14Z

+  /**
+   * Returns the {@link SequenceIdType} corresponding to the provided RocksDB key string, or null if unmapped.
+   */
+  public static SequenceIdType fromDbKey(String dbKey) {


This method is unused except testReturnsNullIfEnumConstantNotAvailable(). Let's remove it and also DB_KEY_MAP.

Indeed, we should always use SequenceIdType and never convert a String to SequenceIdType.

szetszwo · 2026-05-08T18:31:01Z

+  public static final String LOCAL_ID = SequenceIdType.LOCAL_ID.getDbKey();
+  public static final String DEL_TXN_ID = SequenceIdType.DEL_TXN_ID.getDbKey();
+  public static final String CONTAINER_ID = SequenceIdType.CONTAINER_ID.getDbKey();

  // Certificate ID for all services, including root certificates, whose ID
  // were using "rootCertificateId" before.
-  public static final String CERTIFICATE_ID = "CertificateId";
+  public static final String CERTIFICATE_ID = SequenceIdType.CERTIFICATE_ID.getDbKey();
  @Deprecated
-  public static final String ROOT_CERTIFICATE_ID = "rootCertificateId";
+  public static final String ROOT_CERTIFICATE_ID = SequenceIdType.ROOT_CERTIFICATE_ID.getDbKey();


Change them to private.

szetszwo · 2026-05-08T18:34:31Z

  public SequenceIdGenerator(ConfigurationSource conf,
      SCMHAManager scmhaManager, Table<String, Long> sequenceIdTable) {
-    this.sequenceIdToBatchMap = new HashMap<>();
+    this.sequenceIdToBatchMap = new EnumMap<>(SequenceIdType.class);


Make it unmodifiable:

this.sequenceIdToBatchMap = newSequenceIdToBatchMap();

static Map<SequenceIdType, Batch> newSequenceIdToBatchMap() { final EnumMap<SequenceIdType, Batch> map = new EnumMap<>(SequenceIdType.class); for (SequenceIdType type : SequenceIdType.values()) { map.put(type, new Batch()); } return Collections.unmodifiableMap(map); }

szetszwo · 2026-05-08T18:35:14Z

      Batch batch = sequenceIdToBatchMap.computeIfAbsent(
-          sequenceIdName, key -> new Batch());
+          idType, key -> new Batch());


Use get()

final Batch batch = sequenceIdToBatchMap.get(idType);

szetszwo · 2026-05-08T18:37:21Z


        // reload lastId from RocksDB.
-        batch.lastId = stateManager.getLastId(sequenceIdName);
+        batch.lastId = stateManager.getLastId(idType.getDbKey());


Change the parameter to SequenceIdType

Change StateManagerImpl.sequenceIdToLastIdMap key to SequenceIdType

Thanks @sreejasahithi @szetszwo for the suggestion!

I have removed the unused method fromDbKey and DB_KEY_MAP and made all constant under SequenceIdGenerator private alongwith EnumMap as unmodifiable and dis suggested changes in test classes.

I've updated code for StateManagerImpl too , the parameters and map keys to use SequenceIdType instead of Strings. Further , to handle how these new Enums are serialized across the Ratis and safely deserialized from existing RocksDB bytes, I implemented the ScmSequenceIdTypeCodec and registered with ScmCodecFactory. This ensures the Enums translate perfectly into our legacy byte format .

Also referred existing TestPipelineIDCodec to create new unit test for codec and verifies the serialization and deserialzation. In the LegacyStringSequenceIdCodecForTesting class simulated, how Sequence IDs were serialized before the Enum refactoring.

navinko · 2026-05-11T14:18:21Z

Thanks @szetszwo @sreejasahithi for the review , tried addressing review suggestions .
Kindly do review once you get some time.

szetszwo

Change the parameter to SequenceIdType

Change StateManagerImpl.sequenceIdToLastIdMap key to SequenceIdType

@navinko , my suggestions above actually need a lot of changes. Sorry that I was not aware of it!

Could you revert them here and do it in a separated JIRA, revert all the changes for StateManager and StateManagerImpl and keep using String?

szetszwo · 2026-05-11T15:43:22Z

     */
    @Replicate
-    Boolean allocateBatch(String sequenceIdName,
+    Boolean allocateBatch(SequenceIdType idType,


We cannot change the parameter class for compatibility. Otherwise, the SCM in old version cannot talk to the SCM in new version. Let's do it in a separated JIRA.

Thanks @szetszwo will update the PR as suggested .

This reverts commit 0cb4610.

This reverts commit 5c7a108.

…tation" This reverts commit 57d20e9.

szetszwo · 2026-05-12T04:11:10Z

@navinko , thanks for the update! Please remove DB_KEY_MAP since it is not used except for tests.

navinko · 2026-05-12T06:44:09Z

Thanks @szetszwo , reverted all the changes for StateManager and StateManagerImpl and keep using String.

szetszwo · 2026-05-13T19:09:41Z

+/**
+ * Represents the sequence ID types managed by {@link SequenceIdGenerator} and their persisted RocksDB keys.
+ */
+public enum SequenceIdType {


@navinko , Thanks for the update!

After some thoughts, .let's simply use the db key as the enum names. Otherwise, it is confusing to have two different strings.

public enum SequenceIdType { localId, delTxnId, containerId, /** Certificate ID for all services, including root certificates. */ CertificateId, /** @deprecated Use {@link #CERTIFICATE_ID} instead. */ @Deprecated rootCertificateId; }

The change looks good other than that.

Thanks @szetszwo for the review . Agreed , will update the change shortly.

Thanks @szetszwo
I just updated the changes ,tried incorporating the suggestions.

used enum constants same as db key

refactored related code ,test cases and updated required comments .

tested with existing and updated test cases .ran locally with docker secure cluster and verified allocation paths and updated under "How was this patch tested"

CI build progressing

szetszwo

+1 the change looks good.

szetszwo · 2026-05-15T18:14:14Z

@navinko , filed HDDS-15288 for improving StateManagerImpl. Please see if you are interested working on it.

navinko · 2026-05-15T18:19:34Z

@navinko , filed HDDS-15288 for improving StateManagerImpl. Please see if you are interested working on it.

Thanks for the review and merge . Sure will assign myself and start working on it.

HDDS-15145. Use enum for the ID type in SequenceIdGenerator

fa55d42

sreejasahithi reviewed May 8, 2026

View reviewed changes

szetszwo reviewed May 8, 2026

View reviewed changes

navinko added 4 commits May 9, 2026 13:39

Merge branch 'apache:master' into HDDS-15145

2a7263c

Merge branch 'apache:master' into HDDS-15145

a3751d9

HDDS-15145. Updated code with ScmSequenceIdTypeCodec implementation

57d20e9

HDDS-15145. Updated comment

5c7a108

navinko marked this pull request as draft May 10, 2026 16:47

HDDS-15145. Updated comments

0cb4610

navinko marked this pull request as ready for review May 11, 2026 14:16

navinko requested review from sreejasahithi and szetszwo May 11, 2026 14:17

szetszwo reviewed May 11, 2026

View reviewed changes

navinko marked this pull request as draft May 11, 2026 20:18

navinko added 3 commits May 12, 2026 02:50

Revert "HDDS-15145. Updated comments"

e6c414b

This reverts commit 0cb4610.

Revert "HDDS-15145. Updated comment"

67898c4

This reverts commit 5c7a108.

Revert "HDDS-15145. Updated code with ScmSequenceIdTypeCodec implemen…

98658a5

…tation" This reverts commit 57d20e9.

navinko added 2 commits May 12, 2026 11:14

Merge branch 'apache:master' into HDDS-15145

9dcb34b

HDDS-15145. Reverted enum related changes for StateManager and impl

f7aac5b

navinko marked this pull request as ready for review May 12, 2026 06:44

navinko requested a review from szetszwo May 12, 2026 06:48

szetszwo reviewed May 13, 2026

View reviewed changes

navinko added 2 commits May 15, 2026 01:01

Merge branch 'apache:master' into HDDS-15145

d23d48f

HDDS-15145. Kept enum constant same as persisted keys.

56e0f16

szetszwo approved these changes May 14, 2026

View reviewed changes

szetszwo merged commit f0e5925 into apache:master May 15, 2026
48 checks passed

Conversation

navinko commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Please describe your PR in detail:

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

sreejasahithi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sreejasahithi May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinko May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinko commented May 11, 2026

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo commented May 12, 2026

Uh oh!

navinko commented May 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

szetszwo commented May 15, 2026

Uh oh!

navinko commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

navinko commented May 7, 2026 •

edited

Loading

sreejasahithi May 8, 2026 •

edited

Loading

szetszwo May 8, 2026 •

edited

Loading

navinko May 11, 2026 •

edited

Loading