fix: retrying get remote offset and recover from last chunk failures. by frankyn · Pull Request #726 · googleapis/java-storage · GitHub
Skip to content

fix: retrying get remote offset and recover from last chunk failures.#726

Merged
gcf-merge-on-green[bot] merged 7 commits into
masterfrom
improve-retry-reliability
Feb 26, 2021
Merged

fix: retrying get remote offset and recover from last chunk failures.#726
gcf-merge-on-green[bot] merged 7 commits into
masterfrom
improve-retry-reliability

Conversation

@frankyn

@frankyn frankyn commented Feb 24, 2021

Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

I added three things in this PR:

  1. Retries around getting remote offset from GCS
  2. Retrying last chunk failures correctly.
  3. Updated documentation for the retry cases

Fixes #709 #687 ☕️

@product-auto-label product-auto-label Bot added the api: storage Issues related to the googleapis/java-storage API. label Feb 24, 2021
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 24, 2021
@frankyn frankyn changed the title Improve retry reliability fix: retrying get remote offset and recover from last chunk failures. Feb 24, 2021
@codecov

codecov Bot commented Feb 24, 2021

Copy link
Copy Markdown

@frankyn frankyn marked this pull request as ready for review February 24, 2021 19:23
@frankyn frankyn requested a review from a team February 24, 2021 19:23

@BenWhitehead BenWhitehead left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a few minor comments/questions

}

@Test
public void testWriteWithDriftRetryCase10() throws IOException {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: I don't know what case N is,

If easy can we improve these test names? maybe something like
testWriteWithDriftRetry_exceptionWhileWriting

response = httpRequest.execute();
int code = response.getStatusCode();
if (code == 201 || code == 200) {
// Upload completed successfully

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: create constants for compared values instead of commenting

201 -> STATUS_CREATED
200 -> STATUS_OK

sb.append("Not sure what occurred. Here's debugging information:\n");
sb.append("Response:\n").append(ex.toString()).append("\n\n");
throw new StorageException(0, sb.toString());
throw translate(ex);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We don't want to keep the information in the exception message?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: only diff between this block and case 4 is Local vs Remote can we extract a method of this logic so it's easier to keep in sync?

@frankyn frankyn added the automerge Merge the pull request once unit tests and other checks pass. label Feb 26, 2021
@gcf-merge-on-green gcf-merge-on-green Bot merged commit b41b881 into master Feb 26, 2021
@gcf-merge-on-green gcf-merge-on-green Bot deleted the improve-retry-reliability branch February 26, 2021 02:34
@gcf-merge-on-green gcf-merge-on-green Bot removed the automerge Merge the pull request once unit tests and other checks pass. label Feb 26, 2021
gcf-merge-on-green Bot pushed a commit that referenced this pull request Mar 1, 2021
🤖 I have created a release \*beep\* \*boop\* 
---
### [1.113.12](https://www.github.com/googleapis/java-storage/compare/v1.113.11...v1.113.12) (2021-02-26)


### Bug Fixes

* retrying get remote offset and recover from last chunk failures. ([#726](https://www.github.com/googleapis/java-storage/issues/726)) ([b41b881](https://www.github.com/googleapis/java-storage/commit/b41b88109e13b5ebbd0393d1f264225c12876be6))


### Dependencies

* update dependency com.google.api-client:google-api-client to v1.31.2 ([#686](https://www.github.com/googleapis/java-storage/issues/686)) ([6b1f036](https://www.github.com/googleapis/java-storage/commit/6b1f0361376167719ec5456181134136d27d1d3c))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.20.0 ([#732](https://www.github.com/googleapis/java-storage/issues/732)) ([c98413d](https://www.github.com/googleapis/java-storage/commit/c98413df9d9514340aed78b5a4d5e596760bb616))
* update kms.version to v0.87.7 ([#724](https://www.github.com/googleapis/java-storage/issues/724)) ([3229bd8](https://www.github.com/googleapis/java-storage/commit/3229bd860f3a4d700a969aa9e922bbf6b5c1ca10))
* update kms.version to v0.87.8 ([#733](https://www.github.com/googleapis/java-storage/issues/733)) ([a21b75f](https://www.github.com/googleapis/java-storage/commit/a21b75fa846f373970298dd98f8f3520fc2b3c97))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.

google cloud storage: NPE from BlobId.java:119

2 participants