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: StreamWriter hang when we reach the inflight limit control and is doing a retry #799

Merged
merged 6 commits into from Jan 17, 2021

Conversation

yirutang
Copy link
Contributor

@yirutang yirutang commented Jan 14, 2021

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@yirutang yirutang requested a review from as a code owner Jan 14, 2021
@yirutang yirutang requested a review from steffnay Jan 14, 2021
@google-cla google-cla bot added the cla: yes label Jan 14, 2021
@product-auto-label product-auto-label bot added the api: bigquerystorage label Jan 14, 2021
@codecov
Copy link

@codecov codecov bot commented Jan 14, 2021

Codecov Report

Merging #799 (b154fc4) into master (031655b) will increase coverage by 0.49%.
The diff coverage is 80.64%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #799      +/-   ##
============================================
+ Coverage     80.64%   81.13%   +0.48%     
- Complexity      992      996       +4     
============================================
  Files            74       74              
  Lines          5333     5339       +6     
  Branches        398      413      +15     
============================================
+ Hits           4301     4332      +31     
+ Misses          848      837      -11     
+ Partials        184      170      -14     
Impacted Files Coverage Δ Complexity Δ
...e/cloud/bigquery/storage/v1beta2/StreamWriter.java 84.98% <80.64%> (+0.40%) 38.00 <0.00> (ø)
...ery/storage/v1beta1/BaseBigQueryStorageClient.java 72.88% <0.00%> (+1.69%) 22.00% <0.00%> (ø%)
...ud/bigquery/storage/v1/BaseBigQueryReadClient.java 60.97% <0.00%> (+2.43%) 12.00% <0.00%> (ø%)
...gquery/storage/v1beta2/BaseBigQueryReadClient.java 60.97% <0.00%> (+2.43%) 12.00% <0.00%> (ø%)
.../bigquery/storage/v1beta2/BigQueryWriteClient.java 75.38% <0.00%> (+4.61%) 33.00% <0.00%> (ø%)
...bigquery/storage/v1alpha2/BigQueryWriteClient.java 80.51% <0.00%> (+6.49%) 38.00% <0.00%> (ø%)
.../google/cloud/bigquery/storage/v1beta2/Waiter.java 72.94% <0.00%> (+15.29%) 18.00% <0.00%> (+4.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 031655b...b154fc4. Read the comment docs.

} catch (IOException | InterruptedException e) {
LOG.info("Got exception while retrying: " + e.toString());
inflightBatch.onFailure(e);
abortInflightRequests(e);
Copy link
Member

@stephaniewang526 stephaniewang526 Jan 15, 2021

Choose a reason for hiding this comment

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

can we add test coverage for catching these lines?

Copy link
Contributor Author

@yirutang yirutang Jan 15, 2021

Choose a reason for hiding this comment

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

good point. I tried this morning but couldn't figure out a good way to hit the code. I will play around a little more, but could we proceed with this first. Datalfow is waiting for a fix.

Copy link
Member

@stephaniewang526 stephaniewang526 Jan 17, 2021

Choose a reason for hiding this comment

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

SG

@stephaniewang526 stephaniewang526 removed the request for review from steffnay Jan 15, 2021
@stephaniewang526 stephaniewang526 merged commit f8f9770 into googleapis:master Jan 17, 2021
17 of 18 checks passed
gcf-merge-on-green bot pushed a commit that referenced this issue Jan 19, 2021
🤖 I have created a release \*beep\* \*boop\* 
---
### [1.8.5](https://www.github.com/googleapis/java-bigquerystorage/compare/v1.8.4...v1.8.5) (2021-01-17)


### Bug Fixes

* StreamWriter hang when we reach the inflight limit control and is doing a retry ([#799](https://www.github.com/googleapis/java-bigquerystorage/issues/799)) ([f8f9770](https://www.github.com/googleapis/java-bigquerystorage/commit/f8f97701e5ca698a170a1d3b6ecb3886e186f9d5))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@yirutang yirutang deleted the reconnection branch Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants