joseluisll opened a new pull request, #8818: URL: https://github.com/apache/hadoop/pull/8818
### Description of PR JIRA: [HADOOP-20017](https://issues.apache.org/jira/browse/HADOOP-20017) The nightly qbt builds (JDK 17 and JDK 21) report 19 SpotBugs warnings in `hadoop-tools/hadoop-distcp`, which make the spotbugs vote -1 for `hadoop-distcp`, `hadoop-tools` and root, and turn the precommit of other open PRs red. This PR fixes 10 of them in code and adds narrow exclusions for the other 9. Each exclusion matches class, method and bug pattern, and the `EI_EXPOSE_REP2` ones also match the field. The module had no SpotBugs exclude file before, so this adds `hadoop-tools/hadoop-distcp/dev-support/findbugs-exclude.xml` and points `spotbugs-maven-plugin` at it in the module pom. I did not add the module to `dev-support/findbugs-exclude-global.xml`, because that file turns these patterns off for the whole module. | # | Class | Warning | Resolution | |---|---|---|---| | 1 | `SimpleCopyListing` | SS_SHOULD_BE_STATIC `maxRetries` | **Code**: `private static final int MAX_RETRIES = 3` | | 2 | `DiffInfo` | CT_CONSTRUCTOR_THROW `<init>` | **Code**: package-private class made `final` | | 3 | `DynamicInputChunk` | CT_CONSTRUCTOR_THROW `<init>` | **Code**: package-private class made `final` | | 4 | `DynamicInputChunkContext` | CT_CONSTRUCTOR_THROW `<init>` | **Code**: package-private class made `final` | | 5 | `CopyListingFileStatus` | EI_EXPOSE_REP2 `setAclEntries` | **Code**: copies the list, keeping null as null | | 6 | `CopyListingFileStatus` | EI_EXPOSE_REP2 `setXAttrs` | **Code**: copies into a `LinkedHashMap`, keeping null as null and the iteration order | | 7 | `DistCpContext` | EI_EXPOSE_REP2 `setSourcePaths` | **Code**: copies the list, keeping null as null | | 8 | `DistCpContext` | EI_EXPOSE_REP `getSourcePaths` | **Code**: returns an unmodifiable view, or null | | 9 | `DistCpOptions$Builder` | EI_EXPOSE_REP2 `<init>` | **Code**: copies the source path list. `DistCpOptions` is documented as immutable, and `getSourcePaths()` already returns an unmodifiable copy | | 10 | `DistCpOptionSwitch` | EI_EXPOSE_REP `getOption` | **Code**: returns a clone of the enum's shared, JVM-wide CLI `Option`. The only caller is `OptionsParser`, which just adds it to `Options` | | 11 | `CopyListingFileStatus` | EI_EXPOSE_REP `getPermission` | **Excluded**: the `FsPermission` is shared on purpose. One instance is created per copied file, so copying would add an allocation per file and per access | | 12 | `CopyListingFileStatus` | EI_EXPOSE_REP2 `<init>` (field `permission`) | **Excluded**: same reason as #11 | | 13 | `DistCpContext` | EI_EXPOSE_REP2 `<init>` (field `options`) | **Excluded**: `DistCpOptions` is immutable once built | | 14 | `DistCp` | CT_CONSTRUCTOR_THROW `<init>` | **Excluded**: the class is `@InterfaceAudience.Public` and non-final, so making it final would break subclasses | | 15 | `ThrottledInputStream` | CT_CONSTRUCTOR_THROW `<init>` | **Excluded**: public, non-final class; the constructor only `assert`s that the bandwidth is positive | | 16 | `CopyCommitter` | EI_EXPOSE_REP2 `<init>` (field `taskAttemptContext`) | **Excluded**: the MapReduce framework passes this context in and the committer has to keep the same instance | | 17 | `DynamicRecordReader` | EI_EXPOSE_REP2 `initialize` (field `taskAttemptContext`) | **Excluded**: same reason as #16 | | 18 | `WorkReport` | EI_EXPOSE_REP2 `<init>` (field `exception`) | **Excluded**: the report carries the original exception back to the caller on purpose | | 19 | `WorkReport` | EI_EXPOSE_REP `getException` | **Excluded**: same reason as #18 | No public method signature changes. There are two small behaviour changes for callers that kept and later changed a collection they passed in. Changes made to the list after `new DistCpOptions.Builder(list, target)` are no longer seen, which matches the documented immutability of `DistCpOptions`. Changing the list returned by `DistCpContext.getSourcePaths()` now throws `UnsupportedOperationException`; when the paths came from `DistCpOptions` it already did. Nothing in the repository depends on either behaviour. **Expected Yetus result:** the spotbugs vote on the trunk (base) column will still be `-1` ("hadoop-distcp in trunk has 21 extant spotbugs warnings"), because Yetus scores the base branch before applying the patch. Those 21 are the warnings fixed here. The patch column reports `0 new + 0 unchanged - 21 fixed = 0 total`. ### How was this patch tested? - SpotBugs on `hadoop-distcp` (`mvn -pl hadoop-tools/hadoop-distcp spotbugs:spotbugs` / `spotbugs:check`): - before: **21** BugInstances. These are the 19 warnings above; `DynamicInputChunk` and `ThrottledInputStream` each have a second constructor that is reported separately. - after: **0** (`BugInstance size is 0`, `spotbugs:check` passes). - With an empty filter file, the build reports exactly the 10 BugInstances that the 9 entries in the exclude file are meant to cover, so the exclusions don't hide anything else. - Checkstyle (`checkstyle:checkstyle`): no new violations. The module total is unchanged, and the changed test files have no issues. - New unit tests cover the behaviour changes. They all fail against the pre-fix code and pass with it: - `TestCopyListingFileStatus#testAclEntriesAndXAttrsAreCopied` - `TestDistCpOptions#testBuilderCopiesSourcePaths` - `TestDistCpOptions#testContextSourcePaths` - `TestDistCpOptions#testOptionSwitchReturnsCopyOfOption` - The full `hadoop-distcp` unit test suite passes locally (Maven 3.9.15, JDK 21), with 0 failures and 0 errors. The 2 skips are the large-file tests in `TestHDFSContractDistCp`, which are skipped by default. - Yetus precommit (JDK 17 and 21) on this change gives `+1` for test4tests, compile, javac, checkstyle, javadoc, mvnsite, shadedclient, asflicense and the hadoop-distcp unit tests; the only `-1` is the base-branch spotbugs vote described above. ### For code changes: - [x] Does the title of this PR start with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')? - [ ] Object storage: Have the integration tests been executed and the endpoint declared according to the connector-specific documentation? *Not applicable: no object store connector changes.* - [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? *No new dependencies.* - [x] If applicable, have you updated the `LICENSE`, `LICENSE-binary`, `NOTICE-binary` files? *Not applicable.* ### AI Tooling If an AI tool was used: - [x] The PR includes the phrase "Contains content generated by Claude Code" where <tool> is the name of the AI tool used. - [X ] My use of AI contributions follows the ASF legal policy https://www.apache.org/legal/generative-tooling.html Contains content generated by Claude Code. -- 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]
