I'm attaching patch v001. This uses the same parallel test execution techniques that have been effective in hadoop-common and hadoop-hdfs. I updated pom.xml to add the parallel-tests profile and changed some test code to generate testing paths based on the Surefire fork number, so that they're guaranteed to be isolated from each other. HADOOP-12801 is a pre-requisite for this patch.

There are some limitations to this approach. Tests that cover root directory access, multi-part upload, or YARN usage would run into conflicts on shared resources. To handle this, I've moved those tests to a separate Surefire execution step, which runs sequentially instead of parallelized. A future improvement could be to refactor tests related to root out of FileSystemContractBaseTest, so that the non-root tests in that suite can run in parallel. TestS3AFileSystemContract takes ~90 seconds to run, so it would be nice to parallelize that with other suites. I chose not to step outside hadoop-aws for the scope of the current patch though.

While I was editing pom.xml, I also removed an unused dependency on Jackson. This change is backwards-compatible, because clients who had been relying on receiving Jackson via transitive dependency through hadoop-aws will continue to receive it through the dependency chain hadoop-aws -> hadoop-common -> Jackson.

In my environment, plain mvn test runs in ~30 minutes, and mvn -Pparallel-tests test runs in ~13 minutes.

Steve Loughran, would you please review when you get a chance?

+1 overall

This message was automatically generated.

I'm attaching patch v002, adding TestJets3tNativeS3FileSystemContract to the include/exclude lists. This is in anticipation that someday we'll resolve HADOOP-13116 too. It's another test suite that can't run in parallel with others.

+1 overall

This message was automatically generated.

One interesting finding from repeated parallel test runs is that I'm seeing a greater occurrence of failures in TestS3AContractRootDir#testListEmptyRootDirectory, despite the fact that this is one of the test suites that I force to run in serial.

Running org.apache.hadoop.fs.contract.s3a.TestS3AContractRootDir
Tests run: 6, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 9.085 sec <<< FAILURE! - in org.apache.hadoop.fs.contract.s3a.TestS3AContractRootDir
testListEmptyRootDirectory(org.apache.hadoop.fs.contract.s3a.TestS3AContractRootDir)  Time elapsed: 3.041 sec  <<< FAILURE!
java.lang.AssertionError: Deleted file: unexpectedly found s3a://cnauroth-test-aws-s3a/fork-2 as  S3AFileStatus{path=s3a://cnauroth-test-aws-s3a/fork-2; isDirectory=true; modification_time=0; access_time=0; owner=; group=; permission=rwxrwxrwx; isSymlink=false}
	at org.junit.Assert.fail(Assert.java:88)
	at org.apache.hadoop.fs.contract.ContractTestUtils.assertPathDoesNotExist(ContractTestUtils.java:706)
	at org.apache.hadoop.fs.contract.ContractTestUtils.assertDeleted(ContractTestUtils.java:566)
	at org.apache.hadoop.fs.contract.ContractTestUtils.assertDeleted(ContractTestUtils.java:544)
	at org.apache.hadoop.fs.contract.AbstractContractRootDirectoryTest.testListEmptyRootDirectory(AbstractContractRootDirectoryTest.java:134)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:606)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	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.internal.runners.statements.FailOnTimeout$StatementThread.run(FailOnTimeout.java:74)

I do see an intermittent failure of this test even without the parallel test patch, so it's not something introduced by this patch. I think the test is subject to failure because of the eventual consistency behavior of deletes against S3. Reviewing debug logs, in the failure case, I get the "Deleting fake empty directory" message, even though it's definitely not empty at this point. If I put a retry loop around the test, sometimes it fails first and then succeeds on the retry.

It might be interesting to explore if the order of operations in S3AFileSystem#listStatus could be changed to help with this. Maybe the common prefixes could be analyzed first before deciding the key represents a fake empty directory. I'm not sure if it's completely solvable though, because any kind of S3 listing modification is subject to eventual consistency effects. I'm not planning to change anything related to this within the scope of the current patch though.

I think our tests generate race conditions more than most other bits of code do, as they are repeatedly working on the same bits of the objects store and trying to do lots of small operations.

Looks like you are hitting the delayed-listing-consistency problem which S3 has —here on a delete, rather than an addition.

why not have that test not fail immediately if the root dir isn't empty, but sleep and spin for a bit until the dir empties up.

other than that though, LGTM.

Do we need to rely on a new system property, or could we just rely on the test class name being unique? that is this.getClass().getName() could provide the path?

I'm attaching patch v003. This adds the rety loop on TestS3AContractRootDir#testListEmptyRootDirectory. I also took the opportunity to clear some Checkstyle nits in files I'm touching.

Do we need to rely on a new system property, or could we just rely on the test class name being unique? that is this.getClass().getName() could provide the path?

Currently the patch relies on overriding AbstractBondedFSContract#getTestPath as a convenient place to parameterize the test path per unique fork. This layer of the code doesn't have direct access to the test suite class. We could change the contract classes so that the test suite class needs to be passed in during construction, but then every test suite would need to be changed. Overall, I prefer what the current patch is doing, but I could be convinced otherwise if you feel strongly about it.

+1 overall

This message was automatically generated.

+1

It's pretty painful to look at what you've had to do regarding test setup, but I don't see any other workaround. And JS is built into Java6+, so it'll work everywhere

oh, can you tack in a couple of lines in the aws index about how to enable this? thanks

Thank you, Steve. Here is patch v004 with the doc change and a minor rebase after the recent commits.

LGTM

+1

I should warn that I'm seeing errors here; I think inconsistencies may be surfing —but I don't see why that should happen with parallel tests.

Maybe if writes are taking time in the background after a close(), the longer that write takes to complete, the more chance the next operation may fail —and with multiple threads trying to use the same network connection, that's generating the load

I should warn that I'm seeing errors here...

Steve Loughran, here are my observations based on additional experiments.

1. If I configure a dedicated bucket per file system (S3, S3N and S3A), then I consistently get a full pass on parallel test runs. This was the setup I was using when I developed the patch.
2. All of my buckets are in AWS region US-west-2 (Oregon), which is very close to my laptop physically.
3. If I configure the same bucket to be used by the test suites for all 3 file systems, then sometimes I see some flaky behavior. I don't know if this is due to data problems, such as perhaps S3N tests leaving bits behind that confuse S3A later, or if the increased activity on a shared bucket simply surfaces eventual consistency problems with greater likelihood.
4. One test run surfaced a bug, and I have submitted a patch for that on HADOOP-13158.
5. TestS3NContractRootDir#testListEmptyRootDirectory probably needs the same retry loop that we put in place for the same test in TestS3AContractRootDir.

I'll continue to look for ways to stabilize this.

I was only testing TestS3A*, so it's not the cross FS problem...something else