Enable Requester Pays support #5140

jean-philippe-martin · 2018-08-27T23:10:15Z

Google Cloud Storage has "requester pays" buckets. When reading from
those buckets, the requester is billed for the bandwidth (normally, it's
the bucket owner who is billed).

This pull request enables this feature with GATK. By default it is
turned off (so there are no unexpected charges). To turn it on, use the
command line argument "--project-for-requester-pays" (or
"--requester-project") to indicate which project to bill.

Example:

$ ./gatk PrintReads --input $INPUT --output=/tmp/reads.bam
fails with: com.google.cloud.storage.StorageException: Bucket is requester pays bucket but no user project provided.

$ ./gatk PrintReads --input $INPUT --output=/tmp/reads.bam --requester-project=$PROJECT
works

This PR also removes the argumentless version of
setGlobalNIODefaultOptions() because it was confusing (it uses the
default values, not those indicated by the user - usually we'd expect
the user's values to be taken into account).

Google Cloud Storage has "requester pays" buckets. When reading from those buckets, the requester is billed for the bandwidth (normally, it's the bucket owner who is billed). This pull request enables this feature with GATK. By default it is turned off (so there are no unexpected charges). To turn it on, use the command line argument: "--project-for-requester-pays" (or "--requester-project"). Example: $ ./gatk PrintReads --input $INPUT --output=/tmp/reads.bam fails with: com.google.cloud.storage.StorageException: Bucket is requester pays bucket but no user project provided. $ ./gatk PrintReads --input $INPUT --output=/tmp/reads.bam --requester-project=$PROJECT works This PR also removes the argumentless version of setGlobalNIODefaultOptions() because it was confusing (it uses the default values, not those indicated by the user - usually we'd expect the user's values to be taken into account).

jean-philippe-martin · 2018-08-27T23:10:59Z

This will address issue #4828.

codecov-io · 2018-08-28T00:07:12Z

Codecov Report

Merging #5140 into master will increase coverage by 1.247%.
The diff coverage is 90.909%.

@@               Coverage Diff               @@
##              master     #5140       +/-   ##
===============================================
+ Coverage     85.568%   86.814%   +1.247%     
- Complexity     28983     29935      +952     
===============================================
  Files           1810      1813        +3     
  Lines         135251    137671     +2420     
  Branches       15000     15393      +393     
===============================================
+ Hits          115731    119518     +3787     
+ Misses         14170     12682     -1488     
- Partials        5350      5471      +121

Impacted Files	Coverage Δ	Complexity Δ
...ellbender/cmdline/StandardArgumentDefinitions.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...oadinstitute/hellbender/utils/gcs/BucketUtils.java	`80.519% <100%> (+0.519%)`	`40 <3> (ø)`	⬇️
...nstitute/hellbender/utils/gcs/BucketUtilsTest.java	`59.406% <100%> (+3.492%)`	`12 <2> (+1)`	⬆️
...stitute/hellbender/cmdline/CommandLineProgram.java	`83.117% <71.429%> (+1.238%)`	`43 <0> (+1)`	⬆️
...pynumber/CallCopyRatioSegmentsIntegrationTest.java	`95.455% <0%> (-4.545%)`	`4% <0%> (ø)`
...ncotator/mafOutput/MafOutputRendererConstants.java	`98.182% <0%> (-0.838%)`	`2% <0%> (+1%)`
...r/tools/walkers/mutect/Mutect2IntegrationTest.java	`91.429% <0%> (-0.428%)`	`63% <0%> (+3%)`
...rs/variantutils/SelectVariantsIntegrationTest.java	`100% <0%> (ø)`	`85% <0%> (+19%)`	⬆️
...uncotator/dataSources/DataSourceUtilsUnitTest.java	`100% <0%> (ø)`	`11% <0%> (+4%)`	⬆️
...dorientation/CollectF1R2CountsIntegrationTest.java	`100% <0%> (ø)`	`18% <0%> (+2%)`	⬆️
... and 65 more

droazen

Review complete, back to @jean-philippe-martin with some comments.

droazen · 2018-08-31T18:41:32Z

src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineProgram.java

@@ -435,7 +438,7 @@ protected void printSettings() {
        final boolean usingIntelInflater = (BlockGunzipper.getDefaultInflaterFactory() instanceof IntelInflaterFactory && ((IntelInflaterFactory)BlockGunzipper.getDefaultInflaterFactory()).usingIntelInflater());
        logger.info("Inflater: " + (usingIntelInflater ? "IntelInflater": "JdkInflater"));

-        logger.info("GCS max retries/reopens: " + BucketUtils.getCloudStorageConfiguration(NIO_MAX_REOPENS).maxChannelReopens());
+        logger.info("GCS max retries/reopens: " + BucketUtils.getCloudStorageConfiguration(NIO_MAX_REOPENS, "").maxChannelReopens());


Can you add a second logger message here indicating whether requester pays is enabled/disabled, and if it's enabled the project that will be billed?

droazen · 2018-08-31T18:42:06Z

src/main/java/org/broadinstitute/hellbender/cmdline/StandardArgumentDefinitions.java

@@ -92,4 +92,6 @@ private StandardArgumentDefinitions(){}
    public static final String USE_JDK_INFLATER_SHORT_NAME = "jdk-inflater";
    public static final String NIO_MAX_REOPENS_LONG_NAME = "gcs-max-retries";
    public static final String NIO_MAX_REOPENS_SHORT_NAME = "gcs-retries";
+    public static final String NIO_PROJECT_FOR_REQUESTER_PAYS_LONG_NAME = "project-for-requester-pays";
+    public static final String NIO_PROJECT_FOR_REQUESTER_PAYS_SHORT_NAME = "requester-project";


I would eliminate the short name, and just always require the long name for this argument. I think it's important for the word "pays" to always be present :)

droazen · 2018-08-31T18:52:55Z

src/main/java/org/broadinstitute/hellbender/utils/gcs/BucketUtils.java

        return String.join("/", Arrays.copyOfRange(split, 3, split.length));

    }

    /**
-     * Sets NIO_MAX_REOPENS and generous timeouts as the global default.
+     * Sets max_reopens, requester_pays, and generous timeouts as the global default.


Add javadoc for the maxReopens and requesterProject args.

droazen · 2018-08-31T18:56:22Z

src/main/java/org/broadinstitute/hellbender/utils/gcs/BucketUtils.java

+            .maxChannelReopens(maxReopens);
+        if (!Strings.isNullOrEmpty(requesterProject)) {
+            // enable requester pays and indicate who pays
+            builder = builder.autoDetectRequesterPays(true).userProject(requesterProject);


Will a sensible exception be thrown if the requesterProject is invalid?

Not there, no. Only when trying to access the bucket.

droazen · 2018-08-31T18:56:32Z

src/main/java/org/broadinstitute/hellbender/utils/gcs/BucketUtils.java

@@ -374,11 +370,15 @@ public static void setGlobalNIODefaultOptions(int maxReopens) {
    }

    /** The config we want to use. **/
-    public static CloudStorageConfiguration getCloudStorageConfiguration(int maxReopens) {
-        return CloudStorageConfiguration.builder()
+    public static CloudStorageConfiguration getCloudStorageConfiguration(int maxReopens, String requesterProject) {


javadoc for the two method args

droazen · 2018-08-31T19:09:53Z

src/test/java/org/broadinstitute/hellbender/utils/gcs/BucketUtilsTest.java

@@ -17,7 +17,7 @@
 public final class BucketUtilsTest extends GATKBaseTest {

    static {
-        BucketUtils.setGlobalNIODefaultOptions();
+        BucketUtils.setGlobalNIODefaultOptions(20,"");


Can you get the defaults from the GATKConfig instead of hardcoding them here?

droazen · 2018-08-31T19:14:05Z

src/main/java/org/broadinstitute/hellbender/cmdline/StandardArgumentDefinitions.java

@@ -92,4 +92,6 @@ private StandardArgumentDefinitions(){}
    public static final String USE_JDK_INFLATER_SHORT_NAME = "jdk-inflater";
    public static final String NIO_MAX_REOPENS_LONG_NAME = "gcs-max-retries";
    public static final String NIO_MAX_REOPENS_SHORT_NAME = "gcs-retries";
+    public static final String NIO_PROJECT_FOR_REQUESTER_PAYS_LONG_NAME = "project-for-requester-pays";


I think gcs-project-for-requester-pays would be better, and more consistent with the existing retry args.

droazen · 2018-08-31T19:17:36Z

src/test/java/org/broadinstitute/hellbender/utils/gcs/BucketUtilsTest.java

@@ -17,7 +17,7 @@
 public final class BucketUtilsTest extends GATKBaseTest {


Is there some sensible way to unit test the requester-pays support (perhaps using a mock object)?

Well what we have here is parsing the command-line arguments, and setting the cloud storage configuration. The former I pretty much already trust. The latter I added a little test for.

droazen

👍 looks good, thanks @jean-philippe-martin !

jean-philippe-martin · 2018-09-04T18:27:37Z

Thank you @droazen for the review!

droazen self-requested a review August 28, 2018 15:56

droazen self-assigned this Aug 28, 2018

droazen suggested changes Aug 31, 2018

View reviewed changes

droazen assigned jean-philippe-martin and unassigned droazen Aug 31, 2018

reviewer feedback

a8f8069

droazen assigned droazen and unassigned jean-philippe-martin Sep 4, 2018

droazen approved these changes Sep 4, 2018

View reviewed changes

droazen merged commit e6f1c6b into master Sep 4, 2018

droazen deleted the jp_requester_pays branch September 4, 2018 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Requester Pays support #5140

Enable Requester Pays support #5140

jean-philippe-martin commented Aug 27, 2018

jean-philippe-martin commented Aug 27, 2018

codecov-io commented Aug 28, 2018 •

edited

Loading

droazen left a comment

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen Aug 31, 2018

jean-philippe-martin Aug 31, 2018

droazen left a comment

jean-philippe-martin commented Sep 4, 2018

		@@ -17,7 +17,7 @@
		public final class BucketUtilsTest extends GATKBaseTest {

Enable Requester Pays support #5140

Enable Requester Pays support #5140

Conversation

jean-philippe-martin commented Aug 27, 2018

jean-philippe-martin commented Aug 27, 2018

codecov-io commented Aug 28, 2018 • edited Loading

Codecov Report

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

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

droazen left a comment

Choose a reason for hiding this comment

jean-philippe-martin commented Sep 4, 2018

codecov-io commented Aug 28, 2018 •

edited

Loading