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.

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

fix: retry using remote offset #604

Merged
merged 3 commits into from
Nov 13, 2020
Merged

fix: retry using remote offset #604

merged 3 commits into from
Nov 13, 2020

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Nov 9, 2020

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 ☕️

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Nov 9, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 9, 2020
@frankyn frankyn requested a review from JesseLovelace November 9, 2020 08:05
Comment on lines 143 to 156
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),

Choose a reason for hiding this comment

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

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);

Comment on lines 173 to 186
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),

Choose a reason for hiding this comment

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

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
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

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

Impacted file tree graph

@@             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.

@JesseLovelace
Copy link
Contributor

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

@JesseLovelace JesseLovelace marked this pull request as ready for review November 13, 2020 18:40
@JesseLovelace JesseLovelace requested a review from a team as a code owner November 13, 2020 18:40
@JesseLovelace JesseLovelace merged commit 216b52c into master Nov 13, 2020
@JesseLovelace JesseLovelace deleted the raw_download branch November 13, 2020 18:44
@littleK0i
Copy link

littleK0i commented Nov 24, 2020

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
Copy link
Member Author

frankyn commented Nov 24, 2020

Thanks for letting us know, @wildraid!

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

@frankyn
Copy link
Member Author

frankyn commented Dec 11, 2020

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

@littleK0i
Copy link

@frankyn , great news! Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retry Based on Remote Offset
4 participants