HDDS-12983. Validator Registry Changes for Supporting Version based validations by swamirishi · Pull Request #8404 · apache/ozone

swamirishi · 2025-05-06T20:44:18Z

What changes were proposed in this pull request?

Making changes to the ValidatorRegistry class to be able to pull the validators registered and inturn being able to get all the underlying validator methods based on the usage of the registered feature validator based on a specified version, request processing phase and request type efficiently.
This is the last PR for completing the request validator fixes after breaking down
#6932 into smaller PRs

What is the link to the Apache JIRA

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

How was this patch tested?

Adding unit tests

…alidations Change-Id: I939888c989eaec0a62884aa564e2f24a594122ef

Change-Id: Icec10ce7bb452753db0936212b0757c01279b533

Change-Id: I9a010c86d0128da8140fa33b46eaeef9da0dc283

adoroszlai

Thanks @swamirishi for the patch.

adoroszlai · 2025-05-14T09:07:42Z

+ public List<Method> validationsFor(RequestType requestType,
+ RequestProcessingPhase phase,
+ Map<Class<? extends Annotation>, ? extends Versioned> requestVersions) {


Please do not format method signature like this. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters.

adoroszlai · 2025-05-14T09:09:08Z

+ Validator validator, String methodName, Class<ReturnValue> returnValueType) {
+ try {
+ return (ReturnValue) validator.getClass().getMethod(methodName).invoke(validator);


Use returnValueType (currently unused) to cast the result, instead of using unchecked cast (ReturnValue).

adoroszlai · 2025-05-14T09:12:13Z

 processingPhase = RequestProcessingPhase.PRE_PROCESS,
- requestType = Type.CreateBucket
+ requestType = Type.CreateBucket,
+ applyBefore = ClientVersion.BUCKET_LAYOUT_SUPPORT


nit:
Suggested change
applyBefore = ClientVersion.BUCKET_LAYOUT_SUPPORT
applyBefore = ClientVersion.BUCKET_LAYOUT_SUPPORT

adoroszlai · 2025-05-14T09:51:55Z

+ * One validator can be applied in multiple, E.g.
+ * {@link org.apache.hadoop.ozone.om.request.validation.OMClientVersionValidator},
+ * {@link org.apache.hadoop.ozone.om.request.validation.OMLayoutVersionValidator}


I don't think this sentence makes sense.

updated the sentence lmk if it is ok now

adoroszlai · 2025-05-14T09:56:44Z

+ ValidatorRegistry<OzoneManagerProtocolProtos.Type> registry =
+ new ValidatorRegistry<>(OzoneManagerProtocolProtos.Type.class, PACKAGE,
+ Arrays.stream(VersionExtractor.values()).map(VersionExtractor::getValidatorClass).collect(Collectors.toSet()),
+ REQUEST_PROCESSING_PHASES);


Can we avoid duplicating this logic in each test case?

adoroszlai · 2025-05-14T10:01:10Z

+ * @return All the items which has an index value greater than given index value.
+ */
+ public List<T> getItemsEqualToIdx(IDX indexValue) {


Javadoc mismatch ("greater than" vs. EqualTo)

adoroszlai · 2025-05-14T10:02:10Z

+ */
+ private static final class IndexedItems<T, IDX extends Comparable<IDX>> {
+ private final List<T> items;
+ private final TreeMap<IDX, Integer> indexMap;


Declare as interface: NavigableMap.

adoroszlai · 2025-05-14T10:02:56Z

+ .sorted((validatorMethodPair1, validatorMethodPair2) ->
+ Integer.compare(
+ this.getApplyBeforeVersion(validatorMethodPair1.getKey()).version(),
+ this.getApplyBeforeVersion(validatorMethodPair2.getKey()).version()))


Extract this lambda to a static method.

adoroszlai · 2025-05-14T10:06:32Z

+ List<Method> validations = registry.validationsFor(request.getCmdType(), PRE_PROCESS,
+ this.getVersions(request));


nit: remove this. and do not wrap line.

Change-Id: Ibf524d42e3d8535377105a07f8ca3213c8c62199

Change-Id: I67634e5517239bdec3965ed8819f9af6ecf577dd

Change-Id: Idf26bbc7e83877a71f46bafdf0216d70733a92ee

Change-Id: Idecefb392dbf5c01227d7c3702db1781b9edb073

Change-Id: I174cea546d6fd021e58966b3a5bca3672e9d567e # Conflicts: # hadoop-ozone/client/pom.xml # hadoop-ozone/interface-storage/pom.xml # hadoop-ozone/recon-codegen/pom.xml # hadoop-ozone/s3gateway/pom.xml

Change-Id: I132006b2b5a638e7f19d9ec1beb33b332d105e98

github-actions · 2025-11-12T00:05:28Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

github-actions · 2025-11-19T00:05:44Z

Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it.

Change-Id: Ie1076419cae849563efc8a14ac97e5c0c83828a9 # Conflicts: # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMDirectoryCreateRequest.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/file/OMFileCreateRequest.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCommitRequest.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyCreateRequest.java # hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRenameRequest.java

Change-Id: I461d46461905d041a613b101182d24e18a143d5c

adoroszlai · 2025-12-19T13:19:41Z

+ @OMLayoutVersionValidator(
 processingPhase = RequestProcessingPhase.PRE_PROCESS,
- requestType = Type.CreateDirectory
+ requestType = Type.CreateDirectory,
+ applyBefore = OMLayoutFeature.ERASURE_CODED_STORAGE_SUPPORT


Please simply replace the two changed lines instead of moving things around.
- @RequestFeatureValidator(- conditions = ValidationCondition.CLUSTER_NEEDS_FINALIZATION,+ @OMLayoutVersionValidator(+ applyBefore = OMLayoutFeature.ERASURE_CODED_STORAGE_SUPPORT,

adoroszlai · 2025-12-19T13:51:16Z

- <bannedImport>org.apache.hadoop.ozone.om.request.validation.RequestFeatureValidator</bannedImport>
+ <bannedImport>org.apache.hadoop.ozone.om.request.validation.OMLayoutVersionValidator</bannedImport>
+ <bannedImport>org.apache.hadoop.ozone.om.request.validation.OMClientVersionValidator</bannedImport>


Created #9530 to reduce the change that this PR needs to make in POM files. The other PR:
adds the new annotations, since they were already introduced in HDDS-11981
more importantly, removes RequestFeatureValidator from modules where it's not even accessible
So this PR will only need to update POMs for the removal of RequestFeatureValidator, and only in fewer modules.

merged with master branch

Change-Id: Ie6717a5c5e5e6821c2ec8d7abc7a5d4a20d6767f # Conflicts: # hadoop-hdds/client/pom.xml # hadoop-hdds/common/pom.xml # hadoop-hdds/container-service/pom.xml # hadoop-hdds/framework/pom.xml # hadoop-hdds/server-scm/pom.xml # hadoop-ozone/cli-admin/pom.xml # hadoop-ozone/cli-shell/pom.xml # hadoop-ozone/common/pom.xml # hadoop-ozone/csi/pom.xml # hadoop-ozone/freon/pom.xml # hadoop-ozone/insight/pom.xml # hadoop-ozone/recon/pom.xml # hadoop-ozone/tools/pom.xml # pom.xml

Change-Id: Ia2f6792669628c67088b8b5a273fb271476e8848

swamirishi · 2025-12-29T12:50:44Z

@adoroszlai I have addressed all your review comments. Can you take a look at the change now.

adoroszlai · 2026-01-07T19:17:06Z

- OMResponse validatedResponse = response;
+ List<Method> validations = registry.validationsFor(request.getCmdType(), POST_PROCESS, this.getVersions(request));
+
+ OMResponse validatedResponse = response.toBuilder().build();


Why is response being copied here?

just wanted to ensure the validators don't change the original object passed so that the caller can get both the objects. I can remove it if you want me to. It is inconsequential

Yes, please remove, it is unnecessary. OMResponse (as a protobuf message) is immutable, so validators cannot change it.

adoroszlai · 2026-01-10T09:27:46Z

@errose28 @fapifta please take a look

Copilot

Pull request overview

This pull request refactors the request validation framework in Ozone Manager to support version-based validations instead of condition-based validations. The changes replace the ValidationCondition enum and RequestFeatureValidator annotation with a more flexible version-based system using OMClientVersionValidator and OMLayoutVersionValidator annotations.

Changes:

Introduced version-based validator registration using @RegisterValidator meta-annotation
Replaced condition-based validation with version comparison logic in ValidatorRegistry
Updated all validator usages across S3, key, file, and bucket request handlers
Added support for both client version and layout version validators
Implemented IndexedItems data structure for efficient version-range lookups

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
ValidatorRegistry.java	Core refactoring to support version-based validator lookups with IndexedItems for efficient range queries
RequestValidations.java	Updated to extract versions from requests and lookup validators based on version ranges
ValidationCondition.java	Removed - replaced by version-based approach
RequestFeatureValidator.java	Removed - replaced by OMClientVersionValidator and OMLayoutVersionValidator
VersionExtractor.java	New enum to extract client and layout versions from OM requests
package-info.java	Updated documentation to reflect new version-based validation approach
Versioned.java	Added versionComparator() utility method
OzoneManagerRequestHandler.java	Updated validators to use OMClientVersionValidator
S3 multipart request handlers	Updated validators to use new annotations with explicit version parameters
Key/file/bucket request handlers	Updated validators to use new annotations with explicit version parameters
Test files	Updated tests to work with version-based validation logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-13T23:23:00Z

+ * applyBeforeVersion: Enum extending Version
+ * RequestType: Enum signifying the type of request.
+ * RequestProcessingPhase: Signifying if the validator is supposed to run pre or post submitting the request.
+ * Based on the afforementioned parameters a complete map is built which stores the validators in a sorted order of


Typo in "afforementioned". It should be "aforementioned".
Suggested change
* Basedontheafforementionedparametersacompletemapisbuiltwhichstoresthevalidatorsinasortedorderof
* Basedontheaforementionedparametersacompletemapisbuiltwhichstoresthevalidatorsinasortedorderof

Copilot · 2026-01-13T23:23:01Z

- return new EnumMap<>(RequestProcessingPhase.class);
- }
+ /**
+ * Class responsible for maintaining indexs of items. Here each item should have an index corresponding to it.


Typo in the word "indexs". It should be "indexes" or "indices".
Suggested change
* Classresponsibleformaintainingindexsofitems. Hereeachitemshouldhaveanindexcorrespondingtoit.
* Classresponsibleformaintainingindexesofitems. Hereeachitemshouldhaveanindexcorrespondingtoit.

Copilot · 2026-01-13T23:23:01Z

+ applyBefore = OMLayoutFeature.QUOTA,
+ processingPhase = PRE_PROCESS,
+ requestType = CreateVolume)
+ public static OMRequest multiPurposePreProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator(


Typo in the method name "multiPurposePreProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator". The word "CLient" should be "Client" (lowercase 'l').

Copilot · 2026-01-13T23:23:01Z

 assertEquals(1, oldClientValidators.size());
- String expectedMethodName = "multiPurposePostProcessCreateVolumeValidator";
+ assertEquals(1, combinedValidators.size());
+ String expectedMethodName = "multiPurposePostProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator";


Typo in the expected method name "multiPurposePostProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator". The word "CLient" should be "Client" (lowercase 'l').

Copilot · 2026-01-13T23:23:01Z

+ * @param item
+ * @param idx


Missing Javadoc for parameters. The add method should document what the item and idx parameters represent.
Suggested change
* @paramitem
* @paramidx
* @paramitemtheitemtoaddtothiscollection
* @paramidxtheindexassociatedwiththeitem; indicesmustbeprovidedinincreasingorder

Copilot · 2026-01-13T23:23:02Z

+ public static OMRequest multiPurposePreProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator(
 OMRequest req, ValidationContext ctx) {
- fireValidationEvent("multiPurposePreProcessCreateVolumeValidator");
+ fireValidationEvent("multiPurposePreProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator");


Typo in the method name "multiPurposePreProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator". The word "CLient" should be "Client" (lowercase 'l').

Copilot · 2026-01-13T23:23:03Z

- requestType = CreateKey)
- public static OMResponse oldClientPostProcessCreateKeyValidator2(
+ requestType = CreateVolume)
+ public static OMResponse multiPurposePostProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator(


Typo in the method name "multiPurposePostProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator". The word "CLient" should be "Client" (lowercase 'l').

Copilot · 2026-01-13T23:23:03Z

+ public static OMResponse multiPurposePostProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator(
 OMRequest req, OMResponse resp, ValidationContext ctx) {
- fireValidationEvent("oldClientPostProcessCreateKeyValidator2");
+ fireValidationEvent("multiPurposePostProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator");


Typo in the method name "multiPurposePostProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator". The word "CLient" should be "Client" (lowercase 'l').

Copilot · 2026-01-13T23:23:03Z

 assertEquals(1, preFinalizeValidators.size());
 assertEquals(1, newClientValidators.size());
- String expectedMethodName = "multiPurposePreProcessCreateVolumeValidator";
+ String expectedMethodName = "multiPurposePreProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator";


Typo in the expected method name "multiPurposePreProcessCreateVolumeBucketLayoutCLientQuotaLayoutValidator". The word "CLient" should be "Client" (lowercase 'l').

Copilot · 2026-01-13T23:23:03Z

+ *
 * The main reason to avoid validating multiple request types with the same
 * validator, is that these validators have to be simple methods without state
 * any complex validation has to happen in the reql request handling.


Typo in "reql". It should be "real".
Suggested change
* anycomplexvalidationhastohappeninthereqlrequesthandling.
* anycomplexvalidationhastohappenintherealrequesthandling.

github-actions · 2026-02-04T00:06:36Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

github-actions · 2026-02-26T00:11:00Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

adoroszlai · 2026-02-26T08:16:04Z

@errose28 @fapifta can we wrap this up?

github-actions · 2026-04-13T00:12:54Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

github-actions · 2026-04-20T00:13:44Z

Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it.

HDDS-12983. Validator Registry Changes for Supporting Version based v…
2fb399c
…alidations Change-Id: I939888c989eaec0a62884aa564e2f24a594122ef

swamirishi requested review from adoroszlai and fapifta May 6, 2025 20:44

swamirishi added 2 commits May 6, 2025 16:51

HDDS-12983. Revert module move change
16dd2d2
Change-Id: Icec10ce7bb452753db0936212b0757c01279b533

HDDS-12983. Fix Validator registry
de26a00
Change-Id: I9a010c86d0128da8140fa33b46eaeef9da0dc283

adoroszlai reviewed May 14, 2025
View reviewed changes

swamirishi added 3 commits May 20, 2025 08:06

HDDS-12983. Change key to validator map
bf98388
Change-Id: Ibf524d42e3d8535377105a07f8ca3213c8c62199

Merge remote-tracking branch 'apache/master' into HEAD
36cd0d5
Change-Id: I67634e5517239bdec3965ed8819f9af6ecf577dd

HDDS-12983. Address review comments
d4b9b33
Change-Id: Idf26bbc7e83877a71f46bafdf0216d70733a92ee

swamirishi requested a review from sumitagrawl May 20, 2025 15:23

swamirishi added 3 commits May 20, 2025 11:41

HDDS-12983. Fix checkstyle issue
64e0f58
Change-Id: Idecefb392dbf5c01227d7c3702db1781b9edb073

Merge remote-tracking branch 'apache/master' into HEAD
4e18baa
Change-Id: I174cea546d6fd021e58966b3a5bca3672e9d567e # Conflicts: # hadoop-ozone/client/pom.xml # hadoop-ozone/interface-storage/pom.xml # hadoop-ozone/recon-codegen/pom.xml # hadoop-ozone/s3gateway/pom.xml

Resolve merge conflicts
3ffac6c
Change-Id: I132006b2b5a638e7f19d9ec1beb33b332d105e98

github-actionsBot added the stale label Nov 12, 2025

github-actionsBot closed this Nov 19, 2025

swamirishi reopened this Dec 17, 2025

HDDS-12983. Fix checkstyle
3743515
Change-Id: I461d46461905d041a613b101182d24e18a143d5c

github-actionsBot removed the stale label Dec 18, 2025

adoroszlai reviewed Dec 19, 2025
View reviewed changes

swamirishi added 2 commits December 29, 2025 07:38

HDDS-12983. Address review comments
a3a696c
Change-Id: Ia2f6792669628c67088b8b5a273fb271476e8848

swamirishi requested a review from adoroszlai December 29, 2025 14:15

adoroszlai reviewed Jan 7, 2026
View reviewed changes

remove unecessary copy
33deff8

adoroszlai requested a review from errose28 January 10, 2026 09:27

smengcl requested review from Copilot and smengcl January 13, 2026 23:14

Copilotstarted reviewing on behalf of smengcl January 13, 2026 23:15 View session

CopilotAI reviewed Jan 13, 2026
View reviewed changes

github-actionsBot added the stale label Feb 4, 2026

swamirishi removed the stale label Feb 4, 2026

github-actionsBot added the stale label Feb 26, 2026

github-actionsBot removed the stale label Feb 27, 2026

swamirishi marked this pull request as ready for review March 1, 2026 16:26

Merge remote-tracking branch 'origin/master' into HDDS-12983
9543c46

github-actionsBot added the stale label Apr 13, 2026

github-actionsBot closed this Apr 20, 2026

	applyBefore = ClientVersion.BUCKET_LAYOUT_SUPPORT
	applyBefore = ClientVersion.BUCKET_LAYOUT_SUPPORT

		List<Method> validations = registry.validationsFor(request.getCmdType(), PRE_PROCESS,
		this.getVersions(request));

	* Basedontheafforementionedparametersacompletemapisbuiltwhichstoresthevalidatorsinasortedorderof
	* Basedontheaforementionedparametersacompletemapisbuiltwhichstoresthevalidatorsinasortedorderof

	* Classresponsibleformaintainingindexsofitems. Hereeachitemshouldhaveanindexcorrespondingtoit.
	* Classresponsibleformaintainingindexesofitems. Hereeachitemshouldhaveanindexcorrespondingtoit.

- * @paramitem
- * @paramidx
+ * @paramitemtheitemtoaddtothiscollection
+ * @paramidxtheindexassociatedwiththeitem; indicesmustbeprovidedinincreasingorder

	* anycomplexvalidationhastohappeninthereqlrequesthandling.
	* anycomplexvalidationhastohappenintherealrequesthandling.

Uh oh!

Conversation

swamirishi commented May 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

adoroszlai 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!

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!

Choose a reason for hiding this comment

Uh oh!

github-actionsBot commented Nov 12, 2025

Uh oh!

github-actionsBot commented Nov 19, 2025

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!

swamirishi commented Dec 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlaiJan 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!

adoroszlai commented Jan 10, 2026

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

CopilotAIJan 13, 2026

Choose a reason for hiding this comment

Uh oh!

CopilotAIJan 13, 2026

swamirishi commented May 6, 2025•
edited
Loading

adoroszlaiJan 8, 2026•
edited
Loading