HADOOP-17365. Contract test for renaming over existing file is too lenient #2447

adoroszlai · 2020-11-09T16:23:48Z

What changes were proposed in this pull request?

Add some assertions to make sure the result of rename() over existing file
more closely matches expectations defined by contract options
rename-returns-false-if-dest-exists and rename-overwrites-dest.

https://issues.apache.org/jira/browse/HADOOP-17365

How was this patch tested?

Ran related contract tests in Hadoop project:

Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.025 s - in org.apache.hadoop.fs.contract.rawlocal.TestRawlocalContractRename
Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.993 s - in org.apache.hadoop.fs.contract.localfs.TestLocalFSContractRename
Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.654 s - in org.apache.hadoop.fs.contract.hdfs.TestHDFSContractRename
Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 12.14 s - in org.apache.hadoop.fs.contract.router.TestRouterHDFSContractRename
Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 13.295 s - in org.apache.hadoop.fs.contract.router.web.TestRouterWebHDFSContractRename

(also some others for S3A and Azure, but tests in those were skipped).

Also ran Ozone rename contract test with various combinations of these two contract options after overriding testRenameFileOverExistingFile().

…nient Change-Id: Iabccda5f1c150cbb7ccf4596d337f1dd740da37d

hadoop-yetus · 2020-11-09T19:20:12Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 33s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚		0m 0s	test4tests	The patch appears to include 1 new or modified test files.
			_ trunk Compile Tests _
+1 💚	mvninstall	37m 29s		trunk passed
+1 💚	compile	21m 59s		trunk passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚	compile	17m 14s		trunk passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+1 💚	checkstyle	0m 52s		trunk passed
+1 💚	mvnsite	1m 31s		trunk passed
+1 💚	shadedclient	17m 41s		branch has no errors when building and testing our client artifacts.
+1 💚	javadoc	1m 4s		trunk passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚	javadoc	1m 37s		trunk passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+0 🆗	spotbugs	2m 19s		Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	2m 16s		trunk passed
			_ Patch Compile Tests _
+1 💚	mvninstall	0m 55s		the patch passed
+1 💚	compile	19m 18s		the patch passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚	javac	19m 18s		the patch passed
+1 💚	compile	17m 9s		the patch passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+1 💚	javac	17m 9s		the patch passed
-0 ⚠️	checkstyle	0m 51s	/diff-checkstyle-hadoop-common-project_hadoop-common.txt	hadoop-common-project/hadoop-common: The patch generated 1 new + 5 unchanged - 2 fixed = 6 total (was 7)
+1 💚	mvnsite	1m 31s		the patch passed
+1 💚	whitespace	0m 1s		The patch has no whitespace issues.
+1 💚	shadedclient	14m 51s		patch has no errors when building and testing our client artifacts.
+1 💚	javadoc	1m 1s		the patch passed with JDK Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1
+1 💚	javadoc	1m 36s		the patch passed with JDK Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
+1 💚	findbugs	2m 23s		the patch passed
			_ Other Tests _
+1 💚	unit	9m 50s		hadoop-common in the patch passed.
+1 💚	asflicense	0m 56s		The patch does not generate ASF License warnings.
		175m 12s

Subsystem	Report/Notes
Docker	ClientAPI=1.40 ServerAPI=1.40 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2447/1/artifact/out/Dockerfile
GITHUB PR	#2447
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
uname	Linux c5ae75eea207 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `ae7b00a`
Default Java	Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.9+11-Ubuntu-0ubuntu1.18.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_272-8u272-b10-0ubuntu1~18.04-b10
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2447/1/testReport/
Max. process+thread count	3113 (vs. ulimit of 5500)
modules	C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-2447/1/console
versions	git=2.17.1 maven=3.6.0 findbugs=4.1.3
Powered by	Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org

This message was automatically generated.

steveloughran · 2020-11-10T15:38:15Z

makes sense.
you couldn't run the s3a/abfs versions of these tests could you?

adoroszlai · 2020-11-10T15:45:11Z

Thanks @steveloughran for taking a look.

you couldn't run the s3a/abfs versions of these tests could you?

Those were skipped by Maven, due to missing auth-keys.xml I guess. Is there a way to run them locally without real cloud credentials?

steveloughran · 2020-11-11T21:19:28Z

Is there a way to run them locally without real cloud credentials?

Afraid not, except in the special case you have a minio s3 server.

If you don't have them, I'll merge them, and if something does fail then we'll decide whether to revert or address the failure

…nient (#2447) Contributed by Attila Doroszlai. Change-Id: I21c29256b52449b7fea335704b3afa02e39c6a39

adoroszlai · 2020-11-12T06:19:19Z

Thanks @steveloughran for reviewing and committing it.

adoroszlai · 2020-11-12T13:37:39Z

I managed to run ITestS3AContractRename against Ozone S3 and got the following failure:

expected rename(s3a://rename/test/source-256.txt, s3a://rename/test/dest-512.txt) to be rejected with exception, but got false

Inspecting S3AFileSystem, it seems rename file over existing file is intended to return false:

hadoop/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

Lines 1383 to 1386 in fc961b6

    
           } catch (RenameFailedException e) { 
        
             LOG.info("{}", e.getMessage()); 
        
             LOG.debug("rename failure", e); 
        
             return e.getExitCode();

and

hadoop/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java

Lines 1452 to 1458 in fc961b6

    
           // source is a file. The destination must be a directory, 
        
           // empty or not 
        
           if (dstStatus.isFile()) { 
        
             throw new RenameFailedException(src, dst, 
        
                 "Cannot rename onto an existing file") 
        
                 .withExitCode(false); 
        
           }

I think it should be declared in the contract xml.

steveloughran · 2020-11-12T15:57:21Z

Yeah, I'd just seen that breakage too.

[ERROR] Tests run: 22, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 49.852 s <<< FAILURE! - in org.apache.hadoop.fs.contract.s3a.ITestS3AContractRename
[ERROR] testRenameFileOverExistingFile[auth=false](org.apache.hadoop.fs.contract.s3a.ITestS3AContractRename)  Time elapsed: 1.58 s  <<< FAILURE!
java.lang.AssertionError: expected rename(s3a://stevel-ireland/fork-0004/test/source-256.txt, s3a://stevel-ireland/fork-0004/test/dest-512.txt) to be rejected with exception, but got false
	at org.junit.Assert.fail(Assert.java:88)
	at org.apache.hadoop.fs.contract.AbstractContractRenameTest.testRenameFileOverExistingFile(AbstractContractRenameTest.java:131)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:748)

[ERROR] testRenameFileOverExistingFile[auth=true](org.apache.hadoop.fs.contract.s3a.ITestS3AContractRename)  Time elapsed: 1.457 s  <<< FAILURE!
java.lang.AssertionError: expected rename(s3a://stevel-ireland/fork-0004/test/source-256.txt, s3a://stevel-ireland/fork-0004/test/dest-512.txt) to be rejected with exception, but got false
	at org.junit.Assert.fail(Assert.java:88)
	at org.apache.hadoop.fs.contract.AbstractContractRenameTest.testRenameFileOverExistingFile(AbstractContractRenameTest.java:131)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.rules.TestWatcher$1.evaluate(TestWatcher.java:55)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:748)

steveloughran · 2020-11-12T16:00:17Z

Created https://issues.apache.org/jira/browse/HADOOP-17376.

steveloughran · 2020-11-12T17:15:18Z

ok, so the FS contract says rename-over-file should be rejected. S3A doesn't do that, but then we have to worry about delayed consistency, where a newly deleted file may still be found with HEAD, so we have to go with allowing the rename to succeed. Pity

yes, contract.xml should be fixable. If you could supply a patch with the option that'd be great
consider the fact that you've caught where s3a is wrong to be a success of the test case

HADOOP-17365. Contract test for renaming over existing file is too le…

06bff94

…nient Change-Id: Iabccda5f1c150cbb7ccf4596d337f1dd740da37d

adoroszlai self-assigned this Nov 9, 2020

adoroszlai requested a review from steveloughran November 10, 2020 08:39

steveloughran merged commit 6f10a05 into apache:trunk Nov 11, 2020

asfgit pushed a commit that referenced this pull request Nov 11, 2020

HADOOP-17365. Contract test for renaming over existing file is too le…

47131cd

…nient (#2447) Contributed by Attila Doroszlai. Change-Id: I21c29256b52449b7fea335704b3afa02e39c6a39

adoroszlai deleted the HADOOP-17365 branch November 12, 2020 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HADOOP-17365. Contract test for renaming over existing file is too lenient #2447

HADOOP-17365. Contract test for renaming over existing file is too lenient #2447

Uh oh!

adoroszlai commented Nov 9, 2020

Uh oh!

hadoop-yetus commented Nov 9, 2020

Uh oh!

steveloughran commented Nov 10, 2020

Uh oh!

adoroszlai commented Nov 10, 2020

Uh oh!

steveloughran commented Nov 11, 2020

Uh oh!

adoroszlai commented Nov 12, 2020

Uh oh!

adoroszlai commented Nov 12, 2020

Uh oh!

steveloughran commented Nov 12, 2020

Uh oh!

steveloughran commented Nov 12, 2020

Uh oh!

steveloughran commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HADOOP-17365. Contract test for renaming over existing file is too lenient #2447

HADOOP-17365. Contract test for renaming over existing file is too lenient #2447

Uh oh!

Conversation

adoroszlai commented Nov 9, 2020

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

hadoop-yetus commented Nov 9, 2020

Uh oh!

steveloughran commented Nov 10, 2020

Uh oh!

adoroszlai commented Nov 10, 2020

Uh oh!

steveloughran commented Nov 11, 2020

Uh oh!

adoroszlai commented Nov 12, 2020

Uh oh!

adoroszlai commented Nov 12, 2020

Uh oh!

steveloughran commented Nov 12, 2020

Uh oh!

steveloughran commented Nov 12, 2020

Uh oh!

steveloughran commented Nov 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants