smengcl commented on PR #9012: URL: https://github.com/apache/ozone/pull/9012#issuecomment-4931703455
### Code review This is an early-stage design/feedback PR (author's own description: "not intended to be ready for merging"). Reviewing in that spirit. Note that PR #8871 by a different author also implements HDDS-13513 -- the community will need to reconcile which design to pursue before either can land. Found 4 issues: 1. `equals()` compares the wrong fields with `==` instead of `.equals()`, making the method return incorrect results for most inputs. ```java volumeName == that.bucketName && // wrong: reference equality and wrong field pairing ``` https://github.com/apache/ozone/blob/047e955d4b23202bad7f9e0b4a6ac9d8a56608c2/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmCompletedRequestInfo.java#L373-L380 2. `CompletedRequestInfoCleanupService` calls `deleteRange(firstKey, lastKey)` but RocksDB `deleteRange` is end-exclusive (`[first, last)`), so the last record intended for deletion is always skipped. The table grows by 1 row every cleanup cycle. The single-key path (`delete(firstKey)`) is correct; the range path needs `deleteRange(firstKey, keyAfterLast)` or an explicit delete of `lastKey` after the range call. https://github.com/apache/ozone/blob/047e955d4b23202bad7f9e0b4a6ac9d8a56608c2/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/CompletedRequestInfoCleanupService.java#L186-L192 3. Several new files are missing the Apache license header. [`CLAUDE.md`](https://github.com/apache/ozone/blob/047e955d4b23202bad7f9e0b4a6ac9d8a56608c2/CLAUDE.md) requires it on all new files, and `rat.sh` will fail. Affected files include `NotificationCheckpointStrategy.java`, `OMEventListener.java`, `OMEventListenerPluginContext.java`, `LocalFileCheckpointStrategy.java`, `OMEventListenerPluginContextImpl.java`, `OzoneFileCheckpointStrategy.java`, `OMEventListenerNotificationStrategy.java`, `S3EventNotificationBuilder.java`, and `S3EventNotificationStrategy.java`. https://github.com/apache/ozone/blob/047e955d4b23202bad7f9e0b4a6ac9d8a56608c2/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/eventlistener/NotificationCheckpointStrategy.java#L1-L5 4. `S3EventNotification.java` and `DateTimeJsonSerializer.java` carry Amazon copyright headers (`Copyright 2014-2025 Amazon Technologies, Inc.` / `Copyright (c) 2016. Amazon.com, Inc.`) copied from the AWS SDK 1.x. The ASF RAT check will reject these. The PR comment already notes "We may not need to fork this class" -- if the class is needed, it should be rewritten or the provenance documented under ASF IP policy. https://github.com/apache/ozone/blob/047e955d4b23202bad7f9e0b4a6ac9d8a56608c2/hadoop-ozone/ozone-manager-plugins/src/main/java/org/apache/hadoop/ozone/om/eventlistener/s3/S3EventNotification.java#L1-L12 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
