fix: retry using remote offset by frankyn · Pull Request #604 · googleapis/java-storage

frankyn · 2020-11-09T08:05:38Z

Note: @JesseLovelace will be driving this PR to completion as I'll be OOO over the next two weeks. Thanks Jesse!

Ensure the tests and linter pass
Code coverage does not decrease

Fixes #603 ☕️

yoshi-code-bot · 2020-11-09T08:07:44Z

google-cloud-storage/src/test/java/com/google/cloud/storage/BlobWriteChannelTest.java

+    expect(storageRpcMock.writeWithResponse(
+            eq(UPLOAD_ID),
+            capture(capturedBuffer),
+            eq(0),
+            eq(0L),
+            eq(MIN_CHUNK_SIZE),
+            eq(false))).andThrow(exception);
+    expect(storageRpcMock.getCurrentUploadOffset(eq(UPLOAD_ID))).andReturn(0L);
+    expect(storageRpcMock.writeWithResponse(
+            eq(UPLOAD_ID),
+            capture(capturedBuffer),
+            eq(0),
+            eq(0L),
+            eq(MIN_CHUNK_SIZE),


Suggested change

expect(storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(0),

eq(0L),

eq(MIN_CHUNK_SIZE),

eq(false))).andThrow(exception);

expect(storageRpcMock.getCurrentUploadOffset(eq(UPLOAD_ID))).andReturn(0L);

expect(storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(0),

eq(0L),

eq(MIN_CHUNK_SIZE),

expect(

storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(0),

eq(0L),

eq(MIN_CHUNK_SIZE),

eq(false)))

.andThrow(exception);

expect(

storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(0),

eq(0L),

eq(MIN_CHUNK_SIZE),

eq(false)))

.andReturn(null);

yoshi-code-bot · 2020-11-09T08:07:44Z

google-cloud-storage/src/test/java/com/google/cloud/storage/BlobWriteChannelTest.java

+    expect(storageRpcMock.writeWithResponse(
+            eq(UPLOAD_ID),
+            capture(capturedBuffer),
+            eq(0),
+            eq(0L),
+            eq(MIN_CHUNK_SIZE),
+            eq(false))).andThrow(exception);
+    expect(storageRpcMock.getCurrentUploadOffset(eq(UPLOAD_ID))).andReturn(10L);
+    expect(storageRpcMock.writeWithResponse(
+            eq(UPLOAD_ID),
+            capture(capturedBuffer),
+            eq(10),
+            eq(10L),
+            eq(MIN_CHUNK_SIZE - 10),


Suggested change

expect(storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(0),

eq(0L),

eq(MIN_CHUNK_SIZE),

eq(false))).andThrow(exception);

expect(storageRpcMock.getCurrentUploadOffset(eq(UPLOAD_ID))).andReturn(10L);

expect(storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(10),

eq(10L),

eq(MIN_CHUNK_SIZE - 10),

expect(

storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(0),

eq(0L),

eq(MIN_CHUNK_SIZE),

eq(false)))

.andThrow(exception);

expect(

storageRpcMock.writeWithResponse(

eq(UPLOAD_ID),

capture(capturedBuffer),

eq(10),

eq(10L),

eq(MIN_CHUNK_SIZE - 10),

eq(false)))

.andReturn(null);

codecov · 2020-11-09T08:12:20Z

Codecov Report

Merging #604 (d987287) into master (ea25492) will decrease coverage by 0.35%.
The diff coverage is 38.80%.

@@             Coverage Diff              @@
##             master     #604      +/-   ##
============================================
- Coverage     64.68%   64.32%   -0.36%     
+ Complexity      629      621       -8     
============================================
  Files            32       32              
  Lines          5255     5317      +62     
  Branches        510      520      +10     
============================================
+ Hits           3399     3420      +21     
- Misses         1697     1735      +38     
- Partials        159      162       +3

Impacted Files	Coverage Δ	Complexity Δ
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.55% <0.00%> (-0.04%)`	`2.00 <0.00> (ø)`
...va/com/google/cloud/storage/spi/v1/StorageRpc.java	`66.23% <ø> (ø)`	`0.00 <0.00> (ø)`
...ogle/cloud/storage/testing/StorageRpcTestBase.java	`94.11% <0.00%> (-1.89%)`	`48.00 <0.00> (ø)`
...ava/com/google/cloud/storage/BlobWriteChannel.java	`68.00% <61.90%> (-6.61%)`	`11.00 <1.00> (+1.00)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea25492...d987287. Read the comment docs.

…o raw_download

JesseLovelace · 2020-11-13T18:35:41Z

The coverage miss seems to be due to getCurrentUploadOffset in HttpStorageRpc not being tested. This is part of way bigger issue, that entire file doesn't seem to have any coverage. I'm going to go ahead and merge this as is and it should be a follow up issue to get coverage for HttpStorageRpc

littleK0i · 2020-11-24T17:18:19Z

I suspect, this fix did not go so well.

I am now getting following exceptions sometimes:

com.google.cloud.storage.StorageException: java.lang.NullPointerException
Stack trace:
com.google.cloud.storage.StorageException.translateAndThrow(StorageException.java:81)
com.google.cloud.storage.BlobWriteChannel.flushBuffer(BlobWriteChannel.java:180)
com.google.cloud.BaseWriteChannel.flush(BaseWriteChannel.java:112)
com.google.cloud.BaseWriteChannel.write(BaseWriteChannel.java:139)
java.base/java.nio.channels.Channels.writeFullyImpl(Channels.java:74)
java.base/java.nio.channels.Channels.writeFully(Channels.java:97)
java.base/java.nio.channels.Channels$1.write(Channels.java:172)

I think, at very least we should have a better error message.

I use version 1.113.4. I'll try to go back to 1.113.3 and see how it goes.

Thank you.

frankyn · 2020-11-24T17:54:12Z

Thanks for letting us know, @wildraid!

Could you share a snippet of code that you used which caused this error?

frankyn · 2020-12-11T16:58:25Z

@wildraid released a new version with a fix for the issue you raised. Thanks for calling it out!

littleK0i · 2020-12-11T17:04:25Z

@frankyn , great news! Thanks for your help!

product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Nov 9, 2020

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 9, 2020

frankyn requested a review from JesseLovelace November 9, 2020 08:05

frankyn assigned JesseLovelace Nov 9, 2020

yoshi-code-bot reviewed Nov 9, 2020

View reviewed changes

fix: retry using remote offset

e47f1e0

frankyn force-pushed the raw_download branch from 0fdb9d7 to e47f1e0 Compare November 9, 2020 08:09

Frank Natividad and others added 2 commits November 13, 2020 10:32

fix: retry using remote offset

93ee6fd

Merge branch 'raw_download' of github.com:googleapis/java-storage int…

d987287

…o raw_download

JesseLovelace approved these changes Nov 13, 2020

View reviewed changes

JesseLovelace marked this pull request as ready for review November 13, 2020 18:40

JesseLovelace requested a review from a team November 13, 2020 18:40

JesseLovelace merged commit 216b52c into master Nov 13, 2020

JesseLovelace deleted the raw_download branch November 13, 2020 18:44

n12c mentioned this pull request Dec 16, 2020

StorageException: Resumable upload is already complete. #666

Closed

frankyn mentioned this pull request Feb 17, 2021

google cloud storage: NPE from BlobId.java:119 #709

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: retry using remote offset#604

fix: retry using remote offset#604
JesseLovelace merged 3 commits intomasterfrom
raw_download

frankyn commented Nov 9, 2020

Uh oh!

yoshi-code-bot Nov 9, 2020

Uh oh!

yoshi-code-bot Nov 9, 2020

Uh oh!

codecov bot commented Nov 9, 2020 •

edited

Loading

Uh oh!

JesseLovelace commented Nov 13, 2020

Uh oh!

littleK0i commented Nov 24, 2020 •

edited

Loading

Uh oh!

frankyn commented Nov 24, 2020

Uh oh!

frankyn commented Dec 11, 2020

Uh oh!

littleK0i commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

frankyn commented Nov 9, 2020

Uh oh!

yoshi-code-bot Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

yoshi-code-bot Nov 9, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JesseLovelace commented Nov 13, 2020

Uh oh!

littleK0i commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankyn commented Nov 24, 2020

Uh oh!

frankyn commented Dec 11, 2020

Uh oh!

littleK0i commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Nov 9, 2020 •

edited

Loading

littleK0i commented Nov 24, 2020 •

edited

Loading