updating google-cloud-nio to 0.79.0? by lbergelson · Pull Request #5546 · broadinstitute/gatk

lbergelson · 2018-12-21T18:56:39Z

updating google-cloud-nio from 0.62.0 -> 0.74.0
fixes confusing gcloud warning about end user credentials that was output on every start up
improves WARNING: Failed to detect whether we are running on Google Compute Engine. #5447 by reducing the warning to an info

cmnbroad · 2018-12-21T19:43:51Z

@lbergelson looks like RandomDNA.java needs to have it's import updated to com.google.common.annotations.VisibleForTesting

cmnbroad

Seems fine - I assume there is no easy way to write a test for this. Also, which part of this changes the warning level for #5447- is that change in the new google-clou-nio jar ?

lbergelson · 2018-12-21T21:44:02Z

@cmnbroad

The warning -> info change is from googleapis/google-auth-library-java#199
That didn't get rid of the stack trace though, just made it an info trace which is still gross. I made a pr which is merged but not yet released which will move the stack trace to debug.

It should be in 0.75 I think...
googleapis/google-auth-library-java#214

This includes two other prs that I made to fix other output issues in the auth library which made it into the 0.74 release.
googleapis/google-auth-library-java#205
googleapis/google-auth-library-java#207

lbergelson · 2018-12-21T21:44:37Z

The only way to test would be scraping the logs, which we could do... but it seems gross.

cmnbroad · 2018-12-21T21:59:02Z

Yeah, probably not worth it.

cmnbroad

Ok, thx for the updates. LGTM once tests pass.

lbergelson · 2018-12-22T06:31:20Z

There seems to be a change in how gcs recognizes directories that breaks things

Something in Files.isDirectory changed.

Previously:
Files.isDirectory(IOUtils.getPath("gs://hellbender/test/resources/parallel_copy/")) == true
now it's false.

Weirdly, without the slash it's now true where previously it wasn't...

@jean-philippe-martin This seems related to googleapis/google-cloud-java#3775

lbergelson · 2018-12-22T06:42:03Z

Toggling the psuedodirs option doesn't seem to fix the problem...

jean-philippe-martin · 2018-12-24T03:48:19Z

That certainly looks like a bug! I'm off for a few days but will take a look when I return.

jean-philippe-martin · 2018-12-28T17:56:39Z

@lbergelson The situation is rather odd: you seem to have both a file and a folder named "gs://hellbender/test/resources/parallel_copy/". Observe the first line of output:

$ gsutil ls gs://hellbender/test/resources/parallel_copy/
gs://hellbender/test/resources/parallel_copy/
gs://hellbender/test/resources/parallel_copy/bar.txt
gs://hellbender/test/resources/parallel_copy/foo.txt

The file exists and is empty. For evidence, observe that I can cat the file (note how it behaves differently from normal folders):

$ gsutil cat gs://hellbender/test/resources/
CommandException: No URLs matched: gs://hellbender/test/resources/
$ gsutil cat gs://hellbender/test/resources/parallel_copy/
$

So NIO is technically correct, gs://hellbender/test/resources/parallel_copy/ is a file. Did you do anything unusual with regards to this path?

jean-philippe-martin · 2018-12-28T19:32:56Z

The issue linked above indicates a way you could have ended in this twilight zone: apparently this is the default behavior of the GCS web UI and its "create folder" button.

(As you can see in there, we're already working on a fix that will make .isDirectory return true in this case).

droazen · 2019-01-16T21:55:12Z

@lbergelson Should this PR be closed? It seems like we'll need a PR to bump us to a different gcloud release that fixes the issue you ran into here.

lbergelson · 2019-01-18T22:08:56Z

I've bumped this to 0.78.0 which includes a fix for the directory issue, it's not JP's fix so we'll see if it works as expected...

codecov-io · 2019-01-18T22:49:35Z

Codecov Report

Merging #5546 into master will increase coverage by 0.001%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #5546       +/-   ##
===============================================
+ Coverage     87.037%   87.038%   +0.001%     
  Complexity     31728     31728               
===============================================
  Files           1943      1943               
  Lines         146193    146194        +1     
  Branches       16141     16141               
===============================================
+ Hits          127242    127244        +2     
  Misses         13064     13064               
+ Partials        5887      5886        -1

Impacted Files	Coverage Δ	Complexity Δ
...org/broadinstitute/hellbender/utils/RandomDNA.java	`95.556% <ø> (ø)`	`23 <0> (ø)`	⬇️
...oadinstitute/hellbender/utils/gcs/BucketUtils.java	`78.667% <100%> (+0.143%)`	`40 <0> (ø)`	⬇️
...nder/utils/runtime/StreamingProcessController.java	`67.773% <0%> (+0.474%)`	`33% <0%> (ø)`	⬇️

lbergelson · 2019-01-23T20:43:08Z

Bleh... the newest version is broken in a worse way. They fixed a bug where some service providers weren't being shaded correctly, but it broke the shadow jar by accidentally shading the service provider for the actual FileSystemProvider.

lbergelson · 2019-01-23T20:53:58Z

I opened a new issue to track this new bug here: googleapis/google-cloud-java#4403

Hopefully this is an easy fix.

droazen · 2019-01-28T18:58:32Z

A fix for the shading issue has been merged into google-cloud-java, but is not yet released.

ldgauthier · 2019-01-28T19:17:03Z

Does that mean 4.1 is still going to use the NIO fork from your personal repo?

droazen · 2019-01-28T19:18:54Z

@ldgauthier No, we've been on the official google-cloud-java release for a long time now -- our fork is long dead. We're just trying to update to the latest version to fix some edge-case issues with empty files in GCS buckets, but it's not a blocker for the 4.1 release.

ldgauthier · 2019-01-28T19:28:21Z

Excellent! I will update my GATK dockers soon and then I won't have to look at that message any more either.

* updating google-cloud-nio from 0.62.0 -> 0.74.0 * fixes confusing gcloud warning about end user credentials that was output on every start up * improves #5447 by reducing the warning to an info

this includes a potential fix for the directory issue

droazen

👍 one comment, then good to merge

droazen · 2019-02-08T17:16:34Z

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

            // enable requester pays and indicate who pays
            builder = builder.autoDetectRequesterPays(true).userProject(requesterProject);
        }
+        builder.usePseudoDirectories(true);


Can you add a comment explaining what this does?

lbergelson assigned cmnbroad Dec 21, 2018

lbergelson requested a review from cmnbroad December 21, 2018 18:56

cmnbroad reviewed Dec 21, 2018

View reviewed changes

cmnbroad approved these changes Dec 21, 2018

View reviewed changes

jean-philippe-martin mentioned this pull request Dec 28, 2018

GCS NIO readAttributes returns CloudStorageObjectAttributes for directory like objects googleapis/google-cloud-java#3918

Closed

droazen assigned lbergelson and unassigned cmnbroad Jan 16, 2019

lbergelson force-pushed the lb_update_nio branch from aedbbf8 to c69f957 Compare January 23, 2019 18:14

lbergelson changed the title ~~updating google-cloud-nio to 0.74.0~~ updating google-cloud-nio to 0.79.0? Jan 23, 2019

lbergelson added 3 commits February 7, 2019 17:07

updating google-cloud-nio to 0.74.0

6edaa1a

* updating google-cloud-nio from 0.62.0 -> 0.74.0 * fixes confusing gcloud warning about end user credentials that was output on every start up * improves #5447 by reducing the warning to an info

fixed issue with VisibleForTesting annotation

0bb69c9

use pseudodirectories

a65ae3b

lbergelson added 2 commits February 7, 2019 17:07

0.74.0 -> 0.78.0

e466d04

this includes a potential fix for the directory issue

update to 0.80.0

8e0b529

lbergelson force-pushed the lb_update_nio branch from c69f957 to 8e0b529 Compare February 7, 2019 22:08

droazen approved these changes Feb 8, 2019

View reviewed changes

droazen assigned droazen and unassigned lbergelson Feb 8, 2019

add comment

402379a

droazen merged commit 2112b4f into master Feb 8, 2019

droazen deleted the lb_update_nio branch February 8, 2019 21:31

Comments

Conversation

lbergelson commented Dec 21, 2018

Uh oh!

cmnbroad commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmnbroad left a comment

Choose a reason for hiding this comment

Uh oh!

lbergelson commented Dec 21, 2018

Uh oh!

lbergelson commented Dec 21, 2018

Uh oh!

cmnbroad commented Dec 21, 2018

Uh oh!

cmnbroad left a comment

Choose a reason for hiding this comment

Uh oh!

lbergelson commented Dec 22, 2018

Uh oh!

lbergelson commented Dec 22, 2018

Uh oh!

jean-philippe-martin commented Dec 24, 2018

Uh oh!

jean-philippe-martin commented Dec 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jean-philippe-martin commented Dec 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droazen commented Jan 16, 2019

Uh oh!

lbergelson commented Jan 18, 2019

Uh oh!

codecov-io commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lbergelson commented Jan 23, 2019

Uh oh!

lbergelson commented Jan 23, 2019

Uh oh!

droazen commented Jan 28, 2019

Uh oh!

ldgauthier commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droazen commented Jan 28, 2019

Uh oh!

ldgauthier commented Jan 28, 2019

Uh oh!

droazen left a comment

Choose a reason for hiding this comment

Uh oh!

droazen Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cmnbroad commented Dec 21, 2018 •

edited

Loading

jean-philippe-martin commented Dec 28, 2018 •

edited

Loading

jean-philippe-martin commented Dec 28, 2018 •

edited

Loading

codecov-io commented Jan 18, 2019 •

edited

Loading

ldgauthier commented Jan 28, 2019 •

edited

Loading