Skip to content

Conversation

@rich7420
Copy link
Contributor

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?

Copy link
Contributor

@ivandika3 ivandika3 left a 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
Copy link
Contributor

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.

Comment on lines 330 to 338
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;
Copy link
Contributor

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");
Copy link
Member

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert this line

@rich7420
Copy link
Contributor Author

@ivandika3 , @peterxcli thanks a lot for your feedback!!! I've made changes.

@ivandika3
Copy link
Contributor

@rich7420 the checkstyle is still failing: https://github.com/rich7420/ozone/actions/runs/18740162208/job/53454664804

@rich7420
Copy link
Contributor Author

@ivandika3 thanks for your reminding! I've solved the check style error.

@ivandika3
Copy link
Contributor

@rich7420 Thanks for the update. Looks like TestOmUtils is failing https://github.com/rich7420/ozone/actions/runs/18841727879/job/53756665645

@rich7420
Copy link
Contributor Author

Copy link
Contributor

@ivandika3 ivandika3 left a 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.

Copy link
Contributor

@ivandika3 ivandika3 left a 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 ivandika3 added the test label Nov 1, 2025
@rich7420
Copy link
Contributor Author

rich7420 commented Nov 1, 2025

@ivandika3 thanks a lot for your suggestions!

@ivandika3 ivandika3 merged commit 833e955 into apache:master Nov 1, 2025
55 of 56 checks passed
@ivandika3
Copy link
Contributor

Thanks @rich7420 for the patch and @peterxcli for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants