HDFS-13660 Copy file till the source file length during distcp #1404

mukund-thakur · 2019-09-05T10:26:39Z

No description provided.

steveloughran · 2019-09-10T11:51:42Z

I did a detailed review on this through vs.code but it managed to lose them all, which is a pain. I'll go through the code again and add the key technical ones
I can't get yetus to test this, as it doesn't trust you. If you attach the .patch to the HADOOP JIRA I can have a go at forcing jenkins to run it. We need yetus happy.
Yetus will be failing on line length; except when its only a few chars past the 80 mark, please chop down the new 90+ char lines
and where there are new params, new javadocs

I also need to know what happens when a file is truncated. Before it would probably just finish and be happy one read() returned -1. Now it can differentiate between read-all-data and read-less-data. This is good-and we need to take advantage of it by failing when this happens (at the very least, reporting it).Can you add a test which uses truncate() to force this

I have very high expectations on tests, the key one being "if a test fails on jenkins and all you have is the report -is that enough to debug a failure?". The new tests don't meet that requirement yet, which I'll highlight

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyMapper.java

...hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestRetriableFileCopyCommand.java

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/util/TestDistCpUtils.java

hadoop-yetus · 2019-09-10T15:20:11Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	102	Docker mode activated.
		_ Prechecks _
+1	dupname	1	No case conflicting files found.
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
+1	mvninstall	1089	trunk passed
+1	compile	28	trunk passed
+1	checkstyle	28	trunk passed
+1	mvnsite	34	trunk passed
+1	shadedclient	772	branch has no errors when building and testing our client artifacts.
+1	javadoc	23	trunk passed
0	spotbugs	42	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	40	trunk passed
		_ Patch Compile Tests _
+1	mvninstall	28	the patch passed
+1	compile	21	the patch passed
+1	javac	21	the patch passed
-0	checkstyle	21	hadoop-tools/hadoop-distcp: The patch generated 18 new + 308 unchanged - 1 fixed = 326 total (was 309)
+1	mvnsite	24	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	765	patch has no errors when building and testing our client artifacts.
+1	javadoc	19	the patch passed
+1	findbugs	48	the patch passed
		_ Other Tests _
+1	unit	817	hadoop-distcp in the patch passed.
+1	asflicense	31	The patch does not generate ASF License warnings.
		3976

Subsystem	Report/Notes
Docker	Client=19.03.2 Server=19.03.2 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/1/artifact/out/Dockerfile
GITHUB PR	#1404
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux bf01694ede4a 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / dc9abd2
Default Java	1.8.0_222
checkstyle	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/1/artifact/out/diff-checkstyle-hadoop-tools_hadoop-distcp.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/1/testReport/
Max. process+thread count	413 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-distcp U: hadoop-tools/hadoop-distcp
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/1/console
versions	git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by	Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

steveloughran · 2019-09-10T19:16:41Z

checkstyle: ignore the "More than 7 parameters" warning; for the others address them unless the line length is about 81-82 chars and cutting it down would make readability worse. We're not purists about "must fit on a punched card" but really want lines of a width where side-by-side reviews are straightforward. Thanks.

steveloughran · 2019-09-10T19:18:58Z

+one of the warnings on RetriableFileCopyCommand.java:300 is about an existing issue on the line you've edited. You aren't to blame for that -but now is the time to fix it. sorry

mukund-thakur · 2019-09-11T14:01:11Z

I also need to know what happens when a file is truncated. Before it would probably just finish and be happy one read() returned -1. Now it can differentiate between read-all-data and read-less-data. This is good-and we need to take advantage of it by failing when this happens (at the very least, reporting it).Can you add a test which uses truncate() to force this

I checked the behaviour of distcp on a cluster on what happens when truncate call is made while copy is running.

With the current trunk distcp it runs fine and copies the data till a particular length. ( Same as you said above - Distcp will be happy once read() return -1)
With this new patch it fails with blocks not found exception as we are forcing it to read till a particular length. Do you want to write a testcase for this case ??

steveloughran · 2019-09-11T14:58:02Z

With this new patch it fails with blocks not found exception as we are forcing it to read till a particular length. Do you want to write a testcase for this case ?

yes. It's a change in behaviour, but probably a good one. A test will verify that things fail the way we expect -and will continue to do so.

One little concern though: where does that filestatus come from? It's created in the mapper just before the operation, right? As if it's created when the list of files to copy is created (which I doubt...) then the current code handles the case where the file is changed between job schedule and task execution -that would be a visible regression, which we would have to worry about.

mukund-thakur · 2019-09-12T10:25:04Z

yes. It's a change in behaviour, but probably a good one. A test will verify that things fail the way we expect -and will continue to do so.

I tried writing the truncate test case but truncate calls always fails while copy is happening with error. ( Failed to TRUNCATE_FILE /tmp/source/1/3 for DFSClient_NONMAPREDUCE_-2094558173_166 on 127.0.0.1 because DFSClient_NONMAPREDUCE_-209455817
3_166 is already the current lease holder.)

One little concern though: where does that filestatus come from? It's created in the mapper just before the operation, right? As if it's created when the list of files to copy is created (which I doubt...) then the current code handles the case where the file is changed between job schedule and task execution -that would be a visible regression, which we would have to worry about.

The filestatus is created in the mapper. So i think we are good here.

hadoop-yetus · 2019-09-12T10:36:06Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	113	Docker mode activated.
		_ Prechecks _
+1	dupname	0	No case conflicting files found.
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
+1	mvninstall	1368	trunk passed
+1	compile	32	trunk passed
+1	checkstyle	32	trunk passed
+1	mvnsite	34	trunk passed
+1	shadedclient	893	branch has no errors when building and testing our client artifacts.
+1	javadoc	29	trunk passed
0	spotbugs	51	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	49	trunk passed
		_ Patch Compile Tests _
+1	mvninstall	32	the patch passed
+1	compile	24	the patch passed
+1	javac	24	the patch passed
-0	checkstyle	23	hadoop-tools/hadoop-distcp: The patch generated 10 new + 329 unchanged - 1 fixed = 339 total (was 330)
+1	mvnsite	26	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	921	patch has no errors when building and testing our client artifacts.
+1	javadoc	21	the patch passed
+1	findbugs	53	the patch passed
		_ Other Tests _
+1	unit	982	hadoop-distcp in the patch passed.
+1	asflicense	29	The patch does not generate ASF License warnings.
		4779

Subsystem	Report/Notes
Docker	Client=19.03.2 Server=19.03.2 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/2/artifact/out/Dockerfile
GITHUB PR	#1404
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux 3e16e24c3409 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `44850f6`
Default Java	1.8.0_222
checkstyle	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/2/artifact/out/diff-checkstyle-hadoop-tools_hadoop-distcp.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/2/testReport/
Max. process+thread count	412 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-distcp U: hadoop-tools/hadoop-distcp
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/2/console
versions	git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by	Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

hadoop-tools/hadoop-distcp/src/main/java/org/apache/hadoop/tools/util/DistCpUtils.java

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyMapper.java

steveloughran · 2019-09-13T14:51:41Z

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestCopyMapper.java

+          }
+        } catch (IOException | InterruptedException e) {
+            LOG.error("Exception encountered ", e);
+            Assert.fail("Test failed: " + e.getMessage());


throw new AssertionError(Test failed: " + e,e)
Stack traces are too import to throw away

Complete stack trace is already printed using logger.

I think @steveloughran is right, we should log in assert instead of logger.

Okay. I will make the change then.
@steveloughran Do you want me to fix the current 3 checkstyle issues? You said we can ignore some for better readability of code while doing parallel reviews.
Thanks.

I'm not that worried about things where lines become 82, 84, 86 chars wide, because often chopping things down can make things more verbose. the current ones are examples of this.

This issue is still open: we need that stack trace. Or the caught exception is saved to some variable outside the runnable; after the run() we throw that exception if non null

...hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestRetriableFileCopyCommand.java

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/util/TestDistCpUtils.java

hadoop-yetus · 2019-09-19T11:56:54Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
0	reexec	33	Docker mode activated.
		_ Prechecks _
+1	dupname	1	No case conflicting files found.
+1	@author	0	The patch does not contain any @author tags.
+1	test4tests	0	The patch appears to include 5 new or modified test files.
		_ trunk Compile Tests _
+1	mvninstall	1134	trunk passed
+1	compile	24	trunk passed
+1	checkstyle	24	trunk passed
+1	mvnsite	27	trunk passed
+1	shadedclient	785	branch has no errors when building and testing our client artifacts.
+1	javadoc	21	trunk passed
0	spotbugs	40	Used deprecated FindBugs config; considering switching to SpotBugs.
+1	findbugs	37	trunk passed
		_ Patch Compile Tests _
+1	mvninstall	25	the patch passed
+1	compile	19	the patch passed
+1	javac	19	the patch passed
-0	checkstyle	18	hadoop-tools/hadoop-distcp: The patch generated 3 new + 328 unchanged - 1 fixed = 331 total (was 329)
+1	mvnsite	25	the patch passed
+1	whitespace	0	The patch has no whitespace issues.
+1	shadedclient	870	patch has no errors when building and testing our client artifacts.
+1	javadoc	21	the patch passed
+1	findbugs	52	the patch passed
		_ Other Tests _
+1	unit	851	hadoop-distcp in the patch passed.
+1	asflicense	27	The patch does not generate ASF License warnings.
		4097

Subsystem	Report/Notes
Docker	Client=19.03.1 Server=19.03.1 base: https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/3/artifact/out/Dockerfile
GITHUB PR	#1404
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux d470c7e49a39 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/hadoop.sh
git revision	trunk / `28913f7`
Default Java	1.8.0_222
checkstyle	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/3/artifact/out/diff-checkstyle-hadoop-tools_hadoop-distcp.txt
Test Results	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/3/testReport/
Max. process+thread count	362 (vs. ulimit of 5500)
modules	C: hadoop-tools/hadoop-distcp U: hadoop-tools/hadoop-distcp
Console output	https://builds.apache.org/job/hadoop-multibranch/job/PR-1404/3/console
versions	git=2.7.4 maven=3.3.9 findbugs=3.1.0-RC1
Powered by	Apache Yetus 0.10.0 http://yetus.apache.org

This message was automatically generated.

steveloughran · 2019-09-24T10:25:33Z

+1, committed

I did some minor tuning before the commit'

move an import in one of the tests to be in the org.apache.hadoop space
use stringifyException to convert the exception to text

Retested all of the distcp tests after those changes; all good

mukund-thakur · 2019-09-25T07:09:02Z

Thanks @steveloughran for the review. Closing the PR.

...hadoop-distcp/src/test/java/org/apache/hadoop/tools/mapred/TestRetriableFileCopyCommand.java

steveloughran · 2020-01-06T17:12:22Z

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/util/TestDistCpUtils.java

@@ -18,6 +18,7 @@

 package org.apache.hadoop.tools.util;

+import org.apache.hadoop.tools.DistCpConstants;


move into the other distcp imports

hadoop-tools/hadoop-distcp/src/test/java/org/apache/hadoop/tools/util/TestDistCpUtils.java

steveloughran · 2020-01-06T17:15:09Z

sorry, my mistake. Thought this was in and was doing soem more reviews. Ignore that. I'm just trying to run through various outstanding PRs before the 2020 ones come in ..

bshashikant requested a review from steveloughran September 10, 2019 10:11

bshashikant assigned mukund-thakur Sep 10, 2019