HDDS-15145. Use enum for the ID type in SequenceIdGenerator#10211
Open
navinko wants to merge 1 commit intoapache:masterfrom
Open
HDDS-15145. Use enum for the ID type in SequenceIdGenerator#10211navinko wants to merge 1 commit intoapache:masterfrom
navinko wants to merge 1 commit intoapache:masterfrom
Conversation
Comment on lines
+54
to
+59
| 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); | ||
| } |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+87
to
+91
| @Test | ||
| public void testReturnsNullIfEnumConstantNotAvailable() { | ||
| assertNull(SequenceIdType.fromDbKey("unmapped-key-string")); | ||
| assertNull(SequenceIdType.fromDbKey(null)); | ||
| } |
Contributor
There was a problem hiding this comment.
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.
szetszwo
reviewed
May 8, 2026
| /** | ||
| * Returns the {@link SequenceIdType} corresponding to the provided RocksDB key string, or null if unmapped. | ||
| */ | ||
| public static SequenceIdType fromDbKey(String dbKey) { |
Contributor
There was a problem hiding this comment.
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.
Comment on lines
+67
to
+75
| 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(); |
| public SequenceIdGenerator(ConfigurationSource conf, | ||
| SCMHAManager scmhaManager, Table<String, Long> sequenceIdTable) { | ||
| this.sequenceIdToBatchMap = new HashMap<>(); | ||
| this.sequenceIdToBatchMap = new EnumMap<>(SequenceIdType.class); |
Contributor
There was a problem hiding this comment.
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);
}
Comment on lines
117
to
+118
| Batch batch = sequenceIdToBatchMap.computeIfAbsent( | ||
| sequenceIdName, key -> new Batch()); | ||
| idType, key -> new Batch()); |
Contributor
There was a problem hiding this comment.
Use get()
final Batch batch = sequenceIdToBatchMap.get(idType);|
|
||
| // reload lastId from RocksDB. | ||
| batch.lastId = stateManager.getLastId(sequenceIdName); | ||
| batch.lastId = stateManager.getLastId(idType.getDbKey()); |
Contributor
There was a problem hiding this comment.
- Change the parameter to SequenceIdType
- Change StateManagerImpl.sequenceIdToLastIdMap key to SequenceIdType
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
SequenceIdGeneratorare retained and now mapped with Enum .StateManager and StateManagerImpl continue to use String parameters for allocateBatch and getLastId and did not change it to adapt Enum.
Added unit tests for SequenceIdType and updated existing SequenceIdGenerator and RootCARotationManager tests
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:
Successful CI : https://github.com/navinko/ozone/actions/runs/25504128377