HDDS-15370. Change sequenceIdTable to use SequenceIdType#10381
Conversation
|
Hi @szetszwo |
There was a problem hiding this comment.
@navinko , thanks for working on this!
Please see the comments inlined and also
https://issues.apache.org/jira/secure/attachment/13082559/10381_review.patch
| /** | ||
| * Codec to serialize/deserialize {@link SequenceIdType}. | ||
| */ | ||
| public final class SequenceIdTypeCodec implements Codec<SequenceIdType> { |
There was a problem hiding this comment.
- We should cache all the results (instead of encoding to/decoding from string every time)
- Since SequenceIdType is very small, let's move the codec to SequenceIdType.
There was a problem hiding this comment.
Thanks @szetszwo for the review and sharing required patch . It make sense catch all at once in construction by using byteArray and byteBuffer and while reading from SEQUENCE_ID_TYPES map.
| public void testCodecBuffersWithOzoneTestUtil() throws Exception { | ||
| for (SequenceIdType type : SequenceIdType.values()) { | ||
| // Verify codec compatibility with heap and direct byte buffers. | ||
| CodecTestUtil.runTest(enumCodec, type, null, null); |
There was a problem hiding this comment.
Pass also the serializedSize.
There was a problem hiding this comment.
applied the patch .Its being passed now type.getByteArray().length
There was a problem hiding this comment.
Thanks @szetszwo for the review. Tried addressing review suggestions.
Fixed checktyle issue for sequenceIdTypes
Fixed findBug issue for getByteArray
Tested locally with docker-compose by writing some data followed by scm failover and getting key info , found no error in SCM logs.
Attached log file from all 3 scm
HDDS-15370_ALL_SCM_LOGS.txt
CI Build : https://github.com/navinko/ozone/actions/runs/26662954429 (Progressing)
There was a problem hiding this comment.
Thanks @szetszwo .
Please feel free to tag me for any new jira , will be happy to work .
Regards
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
2 participants
What changes were proposed in this pull request?
Refactored the sequenceIdTable from Table<String, Long> to Table<SequenceIdType, Long>, adapting SequenceIdType for our in-memory data structure - sequenceIdTable. To handle this cleanly, I introduced a new SequenceIdTypeCodec implementation that serializes the enum constants to the exact same binary format as the old strings, ensuring absolute compatibility and continuity on disk.
Please describe your PR in detail:
While refactoring noticed SCMMetadataStore.java lives in the framework module and SequenceIdType.java was created inside the server-scm module. To avoid the circular dependencies, moved SequenceIdType and it's test class in framework module.
Created new codec implementation for SequenceIdType- "SequenceIdTypeCodec" and updated SCMDBDefinition by replacing with StringCodec.
Updated SCMMetadataStore and it's implementation with required changes.
Updated SequenceIdGenerator for adapting SequenceIdType under sequenceIdTable.
Added new test for SequenceIdTypeCodec ensuring serialization and desrialization works as it is.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15370
How was this patch tested?
Unit test with new test class-TestSequenceIdTypeCodec
Ran docker-compose locally with secure cluster and validated the logs
HDDS-15370.log
CI build Successful : https://github.com/navinko/ozone/actions/runs/26605659056