- 
                Notifications
    
You must be signed in to change notification settings  - Fork 574
 
HDDS-13822. Add regression testing for OM epoch and txId calculation #9188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rich7420 for the test. Overall LGTM. Please kindly fix the checkstyle failure.
| } | ||
| } | ||
| 
               | 
          ||
| // Regression tests for OM epoch and txld calculation methods | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Let's remove this comment.
| long validTxId = OmUtils.MAX_TRXN_ID; | ||
| // Test valid case - should not throw exception | ||
| try { | ||
| OmUtils.getObjectIdFromTxId(2, validTxId); | ||
| } catch (Exception e) { | ||
| fail("Valid txId should not throw exception: " + e.getMessage()); | ||
| } | ||
| 
               | 
          ||
| long invalidTxId = OmUtils.MAX_TRXN_ID + 1; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the actual value instead of taking from OmUtils.MAX_TRXN_ID
| String serviceId = "myOmId"; | ||
| 
               | 
          ||
| conf.set(OZONE_OM_NODES_KEY + "." + serviceId, "omA,omB,omC"); | ||
| conf.set(OZONE_OM_NODES_KEY + "." + serviceId, "omA,omB,omC"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this line
| 
               | 
          ||
| String serviceId2 = "myOmId2"; | ||
| conf.set(OZONE_OM_NODES_KEY + "." + serviceId2, "om1"); | ||
| conf.set(OZONE_OM_NODES_KEY + "." + serviceId2, "om1"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this line
| 
           @ivandika3 , @peterxcli thanks a lot for your feedback!!! I've made changes.  | 
    
| 
           @rich7420 the checkstyle is still failing: https://github.com/rich7420/ozone/actions/runs/18740162208/job/53454664804  | 
    
| 
           @ivandika3 thanks for your reminding! I've solved the check style error.  | 
    
| 
           @rich7420 Thanks for the update. Looks like   | 
    
This reverts commit b39bbc3.
dffc3f5    to
    ea60cf4      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, left one comment.
        
          
                hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. LGTM +1.
| 
           @ivandika3 thanks a lot for your suggestions!  | 
    
| 
           Thanks @rich7420 for the patch and @peterxcli for the review.  | 
    
What changes were proposed in this pull request?
This PR proposes adding regression tests for the OmUtils class in Apache Ozone. These tests ensure that critical methods handling OM epoch and transaction ID (txId) calculations (specifically getOMEpoch, getObjectIdFromTxId, addEpochToTxId, and getTxIdFromObjectId) maintain their expected behavior and don't introduce regressions with future code changes.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13822
How was this patch tested?