Add ExtremeReadsTest #3070

jean-philippe-martin · 2017-06-08T18:09:52Z

This was a very useful debug tool when working on issue #2685. It sends many parallel reads for a long time. This makes sure that the combination of the cloud provider's throttling and our own retry parameters allows us to eventually read everything to completion and not fail with disconnection errors.

This test is disabled by default, because it takes too long to be run every time. But if there's a doubt about retries we can dust it off and run it.

codecov-io · 2017-06-08T18:54:58Z

Codecov Report

Merging #3070 into master will decrease coverage by 0.095%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #3070       +/-   ##
===============================================
- Coverage     79.721%   79.626%   -0.095%     
- Complexity     18162     18804      +642     
===============================================
  Files           1223      1233       +10     
  Lines          66649     69240     +2591     
  Branches       10409     11069      +660     
===============================================
+ Hits           53133     55133     +2000     
- Misses          9309      9788      +479     
- Partials        4207      4319      +112

Impacted Files	Coverage Δ	Complexity Δ
...institute/hellbender/utils/read/SamComparison.java	`57.403% <0%> (-22.961%)`	`86% <0%> (ø)`
...bender/tools/walkers/annotator/OxoGReadCounts.java	`86.408% <0%> (-9.244%)`	`62% <0%> (+43%)`
...e/hellbender/engine/spark/SparkContextFactory.java	`71.233% <0%> (-2.74%)`	`11% <0%> (ø)`
...bender/tools/walkers/annotator/FragmentLength.java	`100% <0%> (ø)`	`12% <0%> (+5%)`	⬆️
...der/tools/walkers/mutect/M2ArgumentCollection.java	`100% <0%> (ø)`	`1% <0%> (ø)`	⬇️
...otypecaller/HaplotypeCallerArgumentCollection.java	`100% <0%> (ø)`	`4% <0%> (+2%)`	⬆️
...ecaller/AssemblyBasedCallerArgumentCollection.java	`100% <0%> (ø)`	`1% <0%> (ø)`	⬇️
...ellbender/tools/walkers/annotator/BaseQuality.java	`100% <0%> (ø)`	`15% <0%> (+7%)`	⬆️
...bender/tools/walkers/annotator/MappingQuality.java	`100% <0%> (ø)`	`11% <0%> (+5%)`	⬆️
...nder/cmdline/PicardCommandLineProgramExecutor.java	`57.143% <0%> (ø)`	`2% <0%> (?)`
... and 30 more

jean-philippe-martin · 2017-06-16T19:56:51Z

With this new version I'm able to make it fail again; I opened a million channels to read the same file (across 1k threads) and got the error below. Yes I know a million parallel reads on a single file is more than a normal user would issue.

shaded.cloud_nio.com.google.api.client.http.HttpRequest execute
WARNING: exception thrown while executing request
javax.net.ssl.SSLHandshakeException: Remote host closed connection during handshake
	at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:992)
	at sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java:1375)
	at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1403)
	at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:1387)
	at sun.net.www.protocol.https.HttpsClient.afterConnect(HttpsClient.java:559)
	at sun.net.www.protocol.https.AbstractDelegateHttpsURLConnection.connect(AbstractDelegateHttpsURLConnection.java:185)
	at sun.net.www.protocol.http.HttpURLConnection.getOutputStream0(HttpURLConnection.java:1316)
	at sun.net.www.protocol.http.HttpURLConnection.getOutputStream(HttpURLConnection.java:1291)
	at sun.net.www.protocol.https.HttpsURLConnectionImpl.getOutputStream(HttpsURLConnectionImpl.java:250)
	at shaded.cloud_nio.com.google.api.client.http.javanet.NetHttpRequest.execute(NetHttpRequest.java:77)
	at shaded.cloud_nio.com.google.api.client.http.HttpRequest.execute(HttpRequest.java:972)
	at shaded.cloud_nio.com.google.auth.oauth2.ServiceAccountCredentials.refreshAccessToken(ServiceAccountCredentials.java:365)
	at shaded.cloud_nio.com.google.auth.oauth2.OAuth2Credentials.refresh(OAuth2Credentials.java:149)
	at shaded.cloud_nio.com.google.auth.oauth2.OAuth2Credentials.getRequestMetadata(OAuth2Credentials.java:135)
	at shaded.cloud_nio.com.google.auth.http.HttpCredentialsAdapter.initialize(HttpCredentialsAdapter.java:96)
	at com.google.cloud.http.HttpTransportOptions$1.initialize(HttpTransportOptions.java:156)
	at shaded.cloud_nio.com.google.api.client.http.HttpRequestFactory.buildRequest(HttpRequestFactory.java:93)
	at shaded.cloud_nio.com.google.api.client.googleapis.services.AbstractGoogleClientRequest.buildHttpRequest(AbstractGoogleClientRequest.java:300)
	at shaded.cloud_nio.com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeUnparsed(AbstractGoogleClientRequest.java:419)
	at shaded.cloud_nio.com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeUnparsed(AbstractGoogleClientRequest.java:352)
	at shaded.cloud_nio.com.google.api.client.googleapis.services.AbstractGoogleClientRequest.executeMedia(AbstractGoogleClientRequest.java:380)
	at shaded.cloud_nio.com.google.api.services.storage.Storage$Objects$Get.executeMedia(Storage.java:5130)
	at com.google.cloud.storage.spi.v1.HttpStorageRpc.read(HttpStorageRpc.java:494)
	at com.google.cloud.storage.BlobReadChannel$1.call(BlobReadChannel.java:127)
	at com.google.cloud.storage.BlobReadChannel$1.call(BlobReadChannel.java:124)
	at shaded.cloud_nio.com.google.api.gax.retrying.DirectRetryingExecutor.submit(DirectRetryingExecutor.java:93)
	at com.google.cloud.RetryHelper.runWithRetries(RetryHelper.java:49)
	at com.google.cloud.storage.BlobReadChannel.read(BlobReadChannel.java:124)
	at com.google.cloud.storage.contrib.nio.CloudStorageReadChannel.read(CloudStorageReadChannel.java:113)
	at org.broadinstitute.hellbender.utils.nio.SeekableByteChannelPrefetcher$WorkUnit.call(SeekableByteChannelPrefetcher.java:131)
	at org.broadinstitute.hellbender.utils.nio.SeekableByteChannelPrefetcher$WorkUnit.call(SeekableByteChannelPrefetcher.java:104)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)
Caused by: java.io.EOFException: SSL peer shut down incorrectly
	at sun.security.ssl.InputRecord.read(InputRecord.java:505)
	at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java:973)
	... 34 more

droazen

Two comments to address, back to @jean-philippe-martin

droazen · 2017-08-21T19:59:13Z

src/test/java/org/broadinstitute/hellbender/utils/nio/ExtremeReadsTest.java

+     **/
+    @Test(groups={"bucket"}, enabled=false)
+    public void manyParallelReads() throws InterruptedException {
+        List<Thread> threads = new ArrayList<>();


Can you switch to using an ExecutorService for the thread pool here? Instantiating Threads directly is not recommended practice. Could you also ensure that the threads are marked as daemon threads?

Done. Though of course we explicitly want to use all threads at the same time, going against the main use case of thread pools.

droazen · 2017-08-21T19:59:45Z

src/test/java/org/broadinstitute/hellbender/utils/nio/ExtremeReadsTest.java

+ * Stress test for reading lots of data from the cloud using a very small prefetch buffer.
+ * Do not run this too often.
+ */
+public final class ExtremeReadsTest extends BaseTest implements Runnable {


Can you implement a static inner Runnable class, instead of making the test class itself Runnable?

jean-philippe-martin · 2017-08-25T22:33:57Z

@droazen all addressed!

droazen · 2017-09-18T19:32:49Z

@jean-philippe-martin Given that we've finally resolved our intermittent GCS failures at scale, do you think it's still valuable to keep this around, or should this PR be closed?

jean-philippe-martin · 2017-09-18T21:40:37Z

I think we should merge this PR. Our fixes are a sort of workaround the fact that GCS doesn't warm up as quickly as we'd like it to. It's good to have the code to test that again if needed.

droazen · 2017-09-25T19:28:49Z

@jean-philippe-martin Could you please rebase this onto the latest master? This branch needs a git-lfs-related change in order to pass tests. Thanks!

This is necessary to reach a higher number of parallel reads.

Switch from Thread to Executor, Switch from the test class implementing Runnable to using a different class instead.

jean-philippe-martin · 2017-09-25T19:58:55Z

No problem @droazen , rebased.

droazen

Some minor remaining TODOs, then we can merge this.

droazen · 2017-09-27T16:29:29Z

src/test/java/org/broadinstitute/hellbender/utils/nio/ExtremeReadsTest.java

+    static String fname = GCS_GATK_TEST_RESOURCES + "large/CEUTrio.HiSeq.WGS.b37.NA12878.20.21.bam";
+
+    static int THREAD_COUNT = 1000;
+    static int CHANNELS_PER_THREAD = 1000;


Make these constants final

droazen · 2017-09-27T16:29:50Z

src/test/java/org/broadinstitute/hellbender/utils/nio/ExtremeReadsTest.java

+
+    static int THREAD_COUNT = 1000;
+    static int CHANNELS_PER_THREAD = 1000;
+    static volatile int errors = 0;


Make errors an instance variable rather than a mutable static variable.

Actually on second thought, let's not. If it's not static then Runner cannot access it unless we make Runner non-static or manually pass it a reference to the ExtremeReadsTest instance.

droazen · 2017-09-27T16:30:08Z

src/test/java/org/broadinstitute/hellbender/utils/nio/ExtremeReadsTest.java

+                    // EOF
+                    long position = chan.position();
+                    if (size != position) {
+                        System.out.println("Done at wrong position! " + position + " != " + size);


Replace all System.out.println() calls with logger calls.

Done, but now I don't see the output anymore in the IntelliJ GUI.

droazen · 2017-09-27T16:30:55Z

src/test/java/org/broadinstitute/hellbender/utils/nio/ExtremeReadsTest.java

+        errors = 0;
+        final Runner runner = new Runner();
+        for (int i=0; i<THREAD_COUNT; i++) {
+            executor.execute(runner);


Pass in a separate Runner instance on each iteration.

OK, but why? Runner is stateless.

jean-philippe-martin · 2017-10-02T16:51:39Z

@droazen I have a new version up with your changes.

jean-philippe-martin · 2017-10-13T20:41:24Z

Thank you @droazen!

jean-philippe-martin mentioned this pull request Jun 8, 2017

NIO SSL Exception in GenomicsDBImport #2685

Closed

droazen self-assigned this Jun 9, 2017

droazen suggested changes Aug 21, 2017

View reviewed changes

droazen assigned jean-philippe-martin and unassigned droazen Aug 21, 2017

jean-philippe-martin added 4 commits September 25, 2017 12:56

Add ExtremeReadsTest

bc32eb9

Added option for multiple parallel reads per thread

e693762

This is necessary to reach a higher number of parallel reads.

Address review comments

494325a

Switch from Thread to Executor, Switch from the test class implementing Runnable to using a different class instead.

Set threads to daemon.

e9c2f82

jean-philippe-martin force-pushed the jp_extreme_reads_test branch from e3d7be4 to e9c2f82 Compare September 25, 2017 19:56

droazen suggested changes Sep 27, 2017

View reviewed changes

reviewer comments

079214f

droazen approved these changes Oct 13, 2017

View reviewed changes

droazen merged commit ffb47f9 into master Oct 13, 2017

droazen deleted the jp_extreme_reads_test branch October 13, 2017 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ExtremeReadsTest #3070

Add ExtremeReadsTest #3070

jean-philippe-martin commented Jun 8, 2017

codecov-io commented Jun 8, 2017 •

edited

Loading

jean-philippe-martin commented Jun 16, 2017

droazen left a comment

droazen Aug 21, 2017

jean-philippe-martin Aug 25, 2017

droazen Aug 21, 2017

jean-philippe-martin Aug 25, 2017

jean-philippe-martin commented Aug 25, 2017

droazen commented Sep 18, 2017

jean-philippe-martin commented Sep 18, 2017

droazen commented Sep 25, 2017

jean-philippe-martin commented Sep 25, 2017

droazen left a comment

droazen Sep 27, 2017

jean-philippe-martin Oct 2, 2017

droazen Sep 27, 2017

jean-philippe-martin Oct 2, 2017

jean-philippe-martin Oct 2, 2017

droazen Sep 27, 2017

jean-philippe-martin Oct 2, 2017

droazen Sep 27, 2017

jean-philippe-martin Oct 2, 2017

jean-philippe-martin commented Oct 2, 2017

jean-philippe-martin commented Oct 13, 2017

Add ExtremeReadsTest #3070

Add ExtremeReadsTest #3070

Conversation

jean-philippe-martin commented Jun 8, 2017

codecov-io commented Jun 8, 2017 • edited Loading

Codecov Report

jean-philippe-martin commented Jun 16, 2017

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jean-philippe-martin commented Aug 25, 2017

droazen commented Sep 18, 2017

jean-philippe-martin commented Sep 18, 2017

droazen commented Sep 25, 2017

jean-philippe-martin commented Sep 25, 2017

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jean-philippe-martin commented Oct 2, 2017

jean-philippe-martin commented Oct 13, 2017

codecov-io commented Jun 8, 2017 •

edited

Loading