Skip to content

Conversation

@swamirishi
Copy link
Contributor

What changes were proposed in this pull request?

Currently the annotation RequestFeatureValidator only works on a binary conditions which could be cumbersome if we want validators to run only for specific older versions. All validators registered would have to perform this redundant version check which could lead to potential bugs (which already exist) if this check is missed somewhere.
This PR looks into adding an annotion & it's corresponding annotation processor which would enable us to add versions while registering a validator. This annotation would not be used anywhere and would be done in a follow up patch.

What is the link to the Apache JIRA

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

How was this patch tested?

Existing Unit tests and additional unit tests.

… a generic version

Change-Id: I0b86b27642a39595f12aee62638a1ea0f5410b14
Change-Id: I219ce2a548bbf7581c32ae0b2e74e0bf6515d5b9
Change-Id: I3650d42f70bb367d7060d69fee395752224d2970
@swamirishi swamirishi marked this pull request as ready for review December 31, 2024 01:51
@adoroszlai adoroszlai requested a review from errose28 January 3, 2025 14:53
@adoroszlai adoroszlai marked this pull request as draft February 14, 2025 09:52
@adoroszlai adoroszlai self-assigned this Feb 14, 2025
@adoroszlai
Copy link
Contributor

Temporarily converted to draft and assigned to myself, to resolve conflicts.

Change-Id: Ibce9c5811ccb37c672801003a72d0dd6b1cf79f9

# Conflicts:
#	hadoop-hdds/annotations/src/main/java/org/apache/ozone/annotations/OmRequestFeatureValidatorProcessor.java
#	hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/request/validation/RequestProcessingPhase.java
#	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/key/OMAllocateBlockRequest.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/OMKeyRenameRequest.java
#	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysRenameRequest.java
#	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestFeatureValidator.java
#	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/RequestValidations.java
#	hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/validation/ValidatorRegistry.java
#	hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/validation/TestRequestFeatureValidatorProcessor.java
#	hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/validation/TestValidatorRegistry.java
#	hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/validation/TestVersionExtractor.java
#	hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/validation/testvalidatorset1/GeneralValidatorsForTesting.java
Change-Id: Iccae47d20472959f9ab0a6f54c0531a56bd7f3da
@swamirishi swamirishi marked this pull request as ready for review February 18, 2025 08:09
@adoroszlai adoroszlai removed their assignment Feb 18, 2025
@swamirishi
Copy link
Contributor Author

@fapifta Can you review this PR, I am breaking the big PR into smaller ones on @adoroszlai's suggestion

Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

Hi @swamirishi,

sorry for the huge delay on this one, I was mixed up PRs earlier, and did not reviewed this one... I think this set of changes are great so far, I have added some suggestions on the different api docs, I believe those we should fix however I am open to different texts if you have a better wording.
Other than that I think we should be good to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not related to this PR directly, but as we adding a new annotation preprocessor and the second part of the class api doc is copied to the new class, let's address the earlier change in the build in the class' api doc please.

In the current build, the annotation processors are directly specified to only those modules where the annotation is intended to be used, and it happens with the general build. This is also visible in the hadoop-ozone/ozone-manager/pom.xml change. So there is no ServiceLoader in action anymore, and these annotation preprocessors along with the actual enablement of the use of the related annotation is controlled by the pom.xml of all individual modules, while in the project pom it is disabled for all modules that does not enable it.

* and is used to validate the RegisterValidator annotations in the
* codebase, to ensure that the annotated classes have the proper methods returning appropriate object types.
*
* The module is compiled in a different execution via Maven before anything
Copy link
Contributor

Choose a reason for hiding this comment

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

The preprocessor apidoc change for the OmRequestFeatureValidatorProcessor should be applied here also.

* code for any server.
* {@link org.apache.hadoop.ozone.request.validation.RegisterValidator}
* is used to register any validator which has the following methods:
* - applyBefore : Returns an enum which implement {@link org.apache.hadoop.ozone.Versioned}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* - applyBefore : Returns an enum which implement {@link org.apache.hadoop.ozone.Versioned}
* - applyBefore : Returns an enum which implements {@link org.apache.hadoop.ozone.Versioned}

@adoroszlai
Copy link
Contributor

@swamirishi would you like to continue working on this?

@swamirishi
Copy link
Contributor Author

sorry for the huge delay on this one, I was mixed up PRs earlier, and did not reviewed this one... I think this set of changes are great so far, I have added some suggestions on the different api docs, I believe those we should fix however I am open to different texts if you have a better wording.

Thanks @adoroszlai for reminding about this PR. I will address the review comments soon

swamirishi and others added 4 commits April 16, 2025 21:49
Change-Id: I0d1f6faaa948c45f4e7a541f26fe0bed354ef32c
Co-authored-by: Istvan Fajth <fapifta@gmail.com>
Change-Id: Ib45bfefaf167195e49ef6fe0c599b7ff23e702c1
Change-Id: I387a88bb0787c01320c2a3e5051fe58724391db8
@swamirishi
Copy link
Contributor Author

@fapifta I have addressed the review comments to the best of my knowledge. Let me know if I have missed anything.

Change-Id: I27c688cab4b763fa87db1637c51dd71781058602
Change-Id: Ic1301778ecf086d03a840b58ad8ea78d93d347c8
@swamirishi
Copy link
Contributor Author

@fapifta If you think there are anymore review comments to be addressed can I address them in the next patch?

Change-Id: I84d48d25dc18b4d414b1b0db0ebdfce60c006b1a
@swamirishi swamirishi marked this pull request as draft May 5, 2025 21:20
swamirishi added 2 commits May 5, 2025 17:26
Change-Id: I325dcfe0b39e3c174f817b9ff6564ac7399a962f
Change-Id: I2eecb3a540e6f7c1b232038b0ab794522cc00cc9
Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

Hi @swamirishi, ok, let's merge this, but please create a JIRA to track the update and review of the documentation for the annotation processors.

@swamirishi swamirishi marked this pull request as ready for review May 6, 2025 18:22
@swamirishi
Copy link
Contributor Author

Hi @swamirishi, ok, let's merge this, but please create a JIRA to track the update and review of the documentation for the annotation processors.

Sure @fapifta I would be creating a follow up jira to fix usage of this annotation as well.

@swamirishi swamirishi merged commit e71dcf6 into apache:master May 6, 2025
53 checks passed
@swamirishi
Copy link
Contributor Author

Thanks for @adoroszlai & @fapifta for reviewing the PR

vtutrinov pushed a commit to vtutrinov/ozone that referenced this pull request May 13, 2025
… a generic version (apache#7603)

Co-authored-by: Istvan Fajth <fapifta@gmail.com>
aswinshakil added a commit to aswinshakil/ozone that referenced this pull request May 16, 2025
…239-container-reconciliation

Commits: 113 commits

8ceb5c3 Revert "HDDS-11232. Spare InfoBucket RPC call for the FileSystem#getFileStatus calls for the general case. (apache#6988)" (apache#8358)
8ecf4a8 HDDS-12993. Use DatanodeID in ReconNodeManager. (apache#8456)
3e09a11 HDDS-13024. HTTP connect to 0.0.0.0 failed (apache#8439)
c3cb14f HDDS-12914. Bump awssdk to 2.31.40, test ResumableFileDownload (apache#8455)
c6b47e9 HDDS-4677. Document Ozone Ports and Connection End Points (apache#8226)
ae0d757 HDDS-13044. Remove DatanodeDetails#getUuid usages from hdds-common/client/container-service (apache#8462)
2bf3f16 HDDS-12568. Implement MiniOzoneCluster.Service for Recon (apache#8452)
e59d251 HDDS-13046. Add .vscode to .gitignore (apache#8461)
50e6d61 HDDS-13022. Split up exclusive size tracking for key and directory cleanup in SnapshotInfo (apache#8433)
6db647d HDDS-12966. DBDefinitionFactory should not throw InvalidArnException (apache#8454)
fe2875c HDDS-12948. [Snapshot] Increase `ozone.om.fs.snapshot.max.limit` default value to 10k (apache#8376)
215bdc2 HDDS-11904. Fix HealthyPipelineSafeModeRule logic. (apache#8386)
dba7557 HDDS-12989. Throw CodecException for the Codec byte[] methods (apache#8444)
d9c73f0 HDDS-11298. Support owner field in the S3 listBuckets request (apache#8315)
7a5129d HDDS-12810. Check and reserve space atomically in VolumeChoosingPolicy (apache#8360)
d51ccf7 HDDS-13016. Add a getAllNodeCount() method to NodeManager. (apache#8445)
8b27fb9 HDDS-12299. Merge OzoneAclConfig into OmConfig (apache#8383)
96f4f79 HDDS-12501. Remove OMKeyInfo from SST files in backup directory. (apache#8214)
8adb500 HDDS-13021. Make callId unique for each request in AbstractKeyDeletingService (apache#8432)
faeadd6 HDDS-13000. [Snapshot] listSnapshotDiffJobs throws NPE. (apache#8418)
7c957cc HDDS-11954. List keys command gets wrong info from BasicOmKeyInfo (apache#8401)
0697383 HDDS-13027. Workaround for GitHub connection failures (apache#8443)
0d5f933 HDDS-12919. Remove redundant FileLock from ChunkWrite (apache#8435)
395892a HDDS-13012. [Snapshot] Add audit logs for listSnapshotDiffJobs, snapshotDiff and cancelSnapshotDiff. (apache#8425)
d92f0ed HDDS-12653. Add option in `ozone debug log container list` to filter by Health State (apache#8415)
78f8a02 HDDS-12645. Improve output of `ozone debug replicas chunk-info` (apache#8146)
f32470c HDDS-12995. Split ListOptions for better reuse (apache#8413)
cdd1b13 HDDS-13019. Bump jline to 3.30.0 (apache#8428)
1651408 HDDS-13018. Bump common-custom-user-data-maven-extension to 2.0.2 (apache#8429)
8f6222e HDDS-12949. Cleanup SCMSafeModeManager (apache#8390)
adb385f HDDS-13002. Use DatanodeID in ContainerBalancerTask (apache#8421)
2b2a796 HDDS-12969. Use DatanodeID in XceiverClientGrpc (apache#8430)
3c0bafa HDDS-13001. Use DatanodeID in SCMBlockDeletingService. (apache#8420)
8448bc2 HDDS-12187. Use hadoop-dependency-client as parent, not dependency (apache#8416)
582fd03 HDDS-13015. Bump zstd-jni to 1.5.7-3 (apache#8426)
ec12f98 HDDS-12970. Use DatanodeID in Pipeline (apache#8414)
3f48a18 HDDS-12179. Improve Container Log to also capture the Volume Information (apache#7858)
034b497 HDDS-12964. Add minimum required version of Docker Compose to RunningViaDocker page (apache#8385)
c0a6b42 HDDS-12947. Add CodecException (apache#8411)
70949c5 HDDS-11603. Reclaimable Key Filter for Snapshots garbage reclaimation (apache#8392)
08283f3 HDDS-12087. TransactionToDNCommitMap too large causes GC to pause for a long time (apache#8347)
f47df78 HDDS-12958. [Snapshot] Add ACL check regression tests for snapshot operations. (apache#8419)
1cc8445 HDDS-12207. Unify output of ozone debug replicas verify checks (apache#8248)
f087d0b HDDS-12994. Use DatanodeID in ReconSCMDBDefinition. (apache#8417)
d6a7723 HDDS-12776. ozone debug CLI command to list all Duplicate open containers (apache#8409)
c78aeb0 HDDS-12951. EC: Log when falling back to reconstruction read (apache#8408)
412f22d HDDS-12959. Eliminate hdds-hadoop-dependency-server (apache#8384)
df701dc HDDS-12996. Workaround for Docker Compose concurrent map writes (apache#8412)
e6daae4 HDDS-12972. Use DatanodeID in ContainerReplica. (apache#8396)
c1103ae HDDS-12877. Support StorageClass field in the S3 HeadObject request (apache#8351)
4135384 HDDS-12973. Add javadoc for CompactionNode() and make getCompactionNodeGraph return ConcurrentMap (apache#8395)
4f2e13c HDDS-12954. Do not throw IOException for checksum. (apache#8387)
49b8fbd HDDS-12971. Use DatanodeID in Node2PipelineMap (apache#8403)
d2da18f HDDS-12346. Reduce code duplication among TestNSSummaryTask classes (apache#8287)
82b73e3 HDDS-11856. Set DN state machine thread priority higher than command handler thread. (apache#8253)
d4f2734 HDDS-12689. Import BOM for AWS SDK, declare dependencies (apache#8406)
4775e76 HDDS-12975. Fix percentage of blocks deleted in grafana dashboard (apache#8398)
ac0d696 HDDS-12968. [Recon] Fix column visibility issue in Derby during schema upgrade finalization. (apache#8393)
e71dcf6 HDDS-11981. Add annotation for registering feature validator based on a generic version (apache#7603)
254297c HDDS-12562. Reclaimable Directory entry filter for reclaiming deleted directory entries (apache#8055)
4f467c8 HDDS-12978. Remove TestMultipartObjectGet (apache#8400)
a99f207 HDDS-12967. Skip CommonChunkManagerTestCases.testFinishWrite if fuser cannot be started (apache#8389)
d29d76b HDDS-12697. Ozone debug CLI to display details of a single container (apache#8264)
1d1bc88 HDDS-12974. Docker could not parse extra host IP (apache#8397)
7e675d7 HDDS-12053. Make print-log-dag command run locally and offline (apache#8016)
d3faab3 HDDS-12561. Reclaimable Rename entry filter for reclaiming renaming entries (apache#8054)
af1f98c HDDS-10822. Tool to omit raft log in OM. (apache#8154)
3201ca4 HDDS-12952. Make OmSnapshotManager#snapshotLimitCheck thread-safe and consistent (apache#8381)
522c88d HDDS-12963. Clean up io.grpc dependencies (apache#8382)
fa8bd9d HDDS-12916. Support ETag in listObjects response (apache#8356)
fdc77db HDDS-12300. Merge OmUpgradeConfig into OmConfig (apache#8378)
623e144 HDDS-12956. Bump vite to 4.5.14 (apache#8375)
8c8eaf1 HDDS-12944. Reduce timeout for integration check (apache#8374)
40d2e00 HDDS-11141. Avoid log flood due due pipeline close in XceiverServerRatis (apache#8325)
9fe1dba HDDS-12942. Init layout version config should not be public (apache#8373)
452e7aa HDDS-12596. OM fs snapshot max limit is not enforced (apache#8377)
8b095d5 HDDS-12795. Rename heartbeat and first election configuration name (apache#8249)
bee8164 HDDS-12920. Configure log4j to gzip rolled over service log files (apache#8357)
e16a50f HDDS-12934. Split submodule for Freon. (apache#8367)
b1e9511 HDDS-12925. Update datanode volume used space on container deletion (apache#8364)
440bc82 Revert "HDDS-12596. OM fs snapshot max limit is not enforced (apache#8157)"
f345492 HDDS-12596. OM fs snapshot max limit is not enforced (apache#8157)
ee7b1dc HDDS-12901. Introduce EventExecutorMetrics instead of setting the metrics props unsafely (apache#8371)
810e148 HDDS-12939. Remove UnknownPipelineStateException. (apache#8372)
560fcdf HDDS-12728. Add Ozone 2.0.0 to compatibility acceptance tests (apache#8361)
5815a47 HDDS-12933. Remove the table names declared in OmMetadataManagerImpl (apache#8370)
ee32fa5 HDDS-12560. Reclaimable Filter for Snaphost Garbage Collections (apache#8053)
45374ea HDDS-12932. Rewrite OMDBDefinition (apache#8362)
5cb6dd8 HDDS-12575. Set default JUnit5 timeout via property (apache#8348)
8efc0cd HDDS-11633. Delete message body too large, causing SCM to fail writing raft log (apache#8354)
ac9d9fd HDDS-12915. Intermittent failure in testCreatePipelineThrowErrorWithDataNodeLimit (apache#8359)
86039e8 HDDS-12848. Create new submodule for ozone admin (apache#8292)
2d0f8cb HDDS-12833. Remove the CodecRegistry field from DBStoreBuilder (apache#8327)
c71b393 HDDS-12921. UnusedPrivateField violations in tests (apache#8353)
9f3dd01 HDDS-12917. cp: option '--update' doesn't allow an argument (apache#8346)
a14b395 HDDS-12922. Use OMDBDefinition in GeneratorOm and FSORepairTool (apache#8355)
c8a98d6 HDDS-12892. OM Tagging Request incorrectly sets full path as key name for FSO (apache#8345)
ade69e3 HDDS-12649. Include name of volume or bucket in length validation error (apache#8322)
c68308d HDDS-12599. Create an ozone debug CLI command to list all the containers based on final state (apache#8282)
319d5a4 HDDS-12773. bad substitution in bats test (apache#8290)
6f5e02a HDDS-12900. (addendum: fix pmd) Use OMDBDefinition in OmMetadataManagerImpl (apache#8337)
2a7000d HDDS-12900. Use OMDBDefinition in OmMetadataManagerImpl (apache#8337)
9c0c66c HDDS-12915. Mark testCreatePipelineThrowErrorWithDataNodeLimit as flaky
a73e052 HDDS-12907. Enable FieldDeclarationsShouldBeAtStartOfClass PMD rule (apache#8344)
d083f82 HDDS-12905. Move field declarations to start of class in ozone-common (apache#8342)
3f90e1c HDDS-12906. Move field declarations to start of class in ozone-manager module (apache#8343)
403fb97 HDDS-12878. Move field declarations to start of class in tests (apache#8308)
63d5c73 HDDS-12912. Remove deprecated `PipelineManager#closePipeline(Pipeline, boolean)` (apache#8340)
cf1fb88 HDDS-12902. Shutdown executor in CloseContainerCommandHandler and ECReconstructionCoordinator (apache#8341)
825ba02 HDDS-9585. Improve import/export log in ContainerLogger (apache#8330)
b70d35a HDDS-12889. Enable AppendCharacterWithChar PMD rule (apache#8324)
cd308ea HDDS-12904. Move field declarations to start of class in other hdds modules (apache#8336)
4905286 HDDS-12899. Move field declarations to start of class in hdds-server-scm (apache#8332)

Conflicts:
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerLogger.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
	hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReplica.java
	hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java
	hadoop-ozone/recon-codegen/src/main/java/org/apache/ozone/recon/schema/ContainerSchemaDefinition.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants