Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enables setting the google-project that will be charged for requests … #1825

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+19 −6
Open

Enables setting the google-project that will be charged for requests … #1825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08b60e4
Enables setting the google-project that will be charged for requests …
yfarjoun Aug 23, 2022
d319891
- argument needs to be optional since the default is null....
yfarjoun Aug 23, 2022
54be805
responding to review
yfarjoun Aug 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/picard/cmdline/CommandLineProgram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@ public abstract class CommandLineProgram {
@Argument(doc="Google Genomics API client_secrets.json file path.", common = true)
public String GA4GH_CLIENT_SECRETS="client_secrets.json";

@Argument(doc="Google project for access to 'requester pays' buckets and objects.", common = true, optional = true)
public String REQUESTER_PAYS_PROJECT = null;

@ArgumentCollection(doc="Special Arguments that have meaning to the argument parsing system. " +
"It is unlikely these will ever need to be accessed by the command line program")
public Object specialArgumentsCollection = useLegacyParser() ?
Expand Down Expand Up @@ -238,6 +241,16 @@ public int instanceMain(final String[] argv) {
if (System.getProperty("ga4gh.client_secrets") == null) {
System.setProperty("ga4gh.client_secrets", GA4GH_CLIENT_SECRETS);
}

if (PathProvider.GCS.isAvailable){
if (System.getProperty("google_project_requester_pays") == null &&
REQUESTER_PAYS_PROJECT != null) {
System.setProperty("google_project_requester_pays", REQUESTER_PAYS_PROJECT);
}
Log.getInstance(this.getClass()).info(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only be output if the google libraries are loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the libraries are not loaded, what's the meaning of having "requestor pays"? is there something you'd like it to output in that case?

String.format("Will use google project %s for gcs requests.", System.getProperty("google_project_requester_pays")));
}

SamReaderFactory.setDefaultValidationStringency(VALIDATION_STRINGENCY);

// Set the compression level everywhere we can think of
Expand Down
12 changes: 6 additions & 6 deletions src/main/java/picard/nio/GoogleStorageUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@

package picard.nio;


import com.google.cloud.storage.StorageOptions;
import com.google.cloud.storage.contrib.nio.CloudStorageConfiguration;
import com.google.cloud.storage.contrib.nio.CloudStorageFileSystemProvider;
Expand All @@ -50,19 +49,20 @@
class GoogleStorageUtils {

public static void initialize() {
// requester pays support is currently not configured
CloudStorageFileSystemProvider.setDefaultCloudStorageConfiguration(GoogleStorageUtils.getCloudStorageConfiguration(20, null));
final String google_project = System.getProperty("google_project_requester_pays");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this is a bit load-order dependent. Have you checked that the commandline argument overrides the setting in the way you want? I think it's also possible to get the default google project so it's not necessary to specify at all. In gatk we instantiate a preferences object and check if it has a value for the project I think, but I'm not sure if it works or has any weirdness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about we get this simple version to work and if people need something else, it can be added later?


CloudStorageFileSystemProvider.setDefaultCloudStorageConfiguration(GoogleStorageUtils.getCloudStorageConfiguration(20, google_project));
CloudStorageFileSystemProvider.setStorageOptions(GoogleStorageUtils.setGenerousTimeouts(StorageOptions.newBuilder()).build());
}

/** The config we want to use. **/
private static CloudStorageConfiguration getCloudStorageConfiguration(int maxReopens, String requesterProject) {
CloudStorageConfiguration.Builder builder = CloudStorageConfiguration.builder()
private static CloudStorageConfiguration getCloudStorageConfiguration(final int maxReopens, final String requesterProject) {
final CloudStorageConfiguration.Builder builder = CloudStorageConfiguration.builder()
// if the channel errors out, re-open up to this many times
.maxChannelReopens(maxReopens);
if (!Strings.isNullOrEmpty(requesterProject)) {
// enable requester pays and indicate who pays
builder = builder.autoDetectRequesterPays(true).userProject(requesterProject);
builder.autoDetectRequesterPays(true).userProject(requesterProject);
}

// this causes the gcs filesystem to treat files that end in a / as a directory
Expand Down