Repository navigation
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
setTimes(Path, mtime, atime)typically uses-1to mean 'do not change'. If… · NewDistCpUtils.unpackAttributes(...)appears to parse/unpack the preserved-attributes set on every… · New
What changed in this PR
Ensures DistCp chunk-concatenation preserves requested file timestamps (-pt / FileAttribute.TIMES) by restoring mtime/atime after concat, and adds contract tests covering chunked copy timestamp behavior across modes and edge cases.
Changes:
- Restore destination file timestamps after chunk concatenation + checksum validation in
CopyCommitterwhenTIMESis requested. - Add parameterized contract tests validating timestamp preservation for chunked copies across direct/atomic/update modes and various file/directory scenarios.
- Add coverage to ensure other preserved attributes (ACLs/XAttrs/perms/owner/group/etc.) remain correct alongside optional timestamp preservation.
| File | Description |
|---|---|
hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/mapred/CopyCommitter.java |
Restores times after concat/validation when FileAttribute.TIMES is enabled. |
hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/contract/AbstractContractDistCpTest.java |
Adds parameterized contract tests for chunked-copy timestamp preservation and interactions with other attributes. |
Review effort: Lite
Copilot stopped reviewing on behalf of
ayushtkn due to an error
October 23, 2026 11:17
|
💔 -1 overall
This message was automatically generated. |
ayushtkn
reviewed
Oct 23, 2026
ayushtkn
left a comment
ayushtkn
left a comment
Member
There was a problem hiding this comment.
Thanx @OneSizeFitsQuorum, can u look at the comments from copilot
There was a problem hiding this comment.
🟡 Changes recommended
The new shared tests admit ADL despite its fixed block size, causing the small multi-block fixtures to fail before DistCp runs.
1 open issue
4 resolved since last review
setTimes(Path, mtime, atime)typically uses-1to mean 'do not change'. If…DistCpUtils.unpackAttributes(...)appears to parse/unpack the preserved-attributes set on every…
Review effort: Balanced
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Member
|
@OneSizeFitsQuorum do you have a JiraId to which the ticket can be assigned, can you let me know your Jira Id or assign the Jira to your name |
Contributor
Author
@ayushtkn Already replied
|
OneSizeFitsQuorum
deleted the
HADOOP-17611-distcp-preserve-chunk-times
branch
October 28, 2026 08:06
Contributor
Author
|
@ayushtkn Thanks a lot for the fast review and merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of PR
HADOOP-17611
With
-blocksperchunkand-pt, CopyMapper preserves timestamps on the chunks, but the subsequent concat in CopyCommitter changes the final file's modification time. Restore the source modification and access times after concatenation, rename, and checksum validation, only whenFileAttribute.TIMESis requested. Parse the TIMES option once before processing the listing.Restore only timestamps: file listing entries omit extended ACLs, so replaying all attributes here can erase ACLs already preserved by the mapper. Directory preservation retains its existing copy scope. Source atime zero remains a valid timestamp, consistent with existing DistCp semantics; it must not be replaced with the -1 sentinel.
Add 14 HDFS contract cases covering TIMES enabled/disabled, ordinary/direct/atomic copies, actual multi-block files, small/empty files, directory scope, update/skip behavior, and preservation of other attributes. A separate committer test verifies restoring epoch-zero atime over nonzero chunk atimes.
The shared tests require explicit fixture support for small blocks and stable source atimes, currently enabled by the HDFS fixture. This prevents fixed-block-size connectors such as ADL from failing during fixture creation. HDFS fixtures use fixed timestamps and disable automatic read-driven atime updates while retaining explicit setTimes support.
How was this patch tested?
After merging Apache Hadoop trunk at
d760975ecbfd56174d13e240ef2beebe70288fc2, ran the Maven reactor with JDK 17:mvn -B -pl hadoop-tools/hadoop-distcp -am \ -Dtest=TestHDFSContractDistCp,TestLocalContractDistCp,TestDistCpSystem,TestCopyMapper,TestCopyCommitter,TestDistCpWithAcls,TestDistCpWithXAttrs \ -Dmaven.test.failure.ignore=false \ -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false \ -Dcheckstyle.skip -Drat.skip=true test100 tests: 84 passed, 16 skipped, zero failures or errors. The skips are 14 unsupported local-filesystem cases and two existing disabled HDFS tests. All 14 HDFS chunk-copy cases and the epoch-zero committer regression passed. The run used temporary Maven cache/mirror settings.
Separately checked all four changed files with the repository Checkstyle configuration: no findings on PR-added lines.
git diff --checkpassed. Cloud live tests were not run.Earlier temporary negative controls confirmed that the original implementation fails the three TIMES-enabled copy-mode cases, and substituting -1 for zero atime fails the new committer test. Other controls detected unconditional timestamp preservation, out-of-scope parent preservation, and ACL loss from replaying all attributes.
Scope note: ordinary chunked single-file copying to a nonexistent destination filename has a separate staging-path failure, reproduced with both original and patched implementations. Tests cover ordinary file-to-existing-directory and direct file-to-new-filename copying. This PR does not change staging paths or optimize mapper-side timestamp calls.
For code changes: