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: Add unit test for concurrent issues we worried about, and fix some locking issues #854

Merged
merged 40 commits into from Mar 2, 2021

Conversation

yirutang
Copy link
Contributor

@yirutang yirutang commented Feb 19, 2021

  • Improve FakeBigQueryService so that we can remove wait code in test.
  • Add unit tests for:
    Have something in inflight queue and we failed and then shutdown.
    Shutdown while append is going on.
    Shutdown with stuff both in batch leftover and inflight queue.
    Additional Batch test to increase code coverage.
  • Fix the locks in:
    Merge AppendAndRefreshAppendLock and MessageBatchLock into one.
    Release lock while we are waiting on waiter.
    Change shutdown and setAlarm to be normal variable since they are all protected by appendAndRefreshAppend lock.
    Make sure inflight requests are flushed after a stream error (on shutdown)

@yirutang yirutang requested a review from as a code owner Feb 19, 2021
@yirutang yirutang requested a review from steffnay Feb 19, 2021
@product-auto-label product-auto-label bot added the api: bigquerystorage label Feb 19, 2021
@google-cla google-cla bot added the cla: yes label Feb 19, 2021
@yirutang yirutang marked this pull request as draft Feb 19, 2021
@yirutang yirutang marked this pull request as ready for review Feb 19, 2021
@yirutang yirutang changed the title fix: Add unit test for concurrent issues we worried about, and fix some locking issues. fix: Add unit test for concurrent issues we worried about, and fix some locking issues Feb 19, 2021
@yirutang yirutang marked this pull request as draft Feb 19, 2021
@yirutang yirutang requested a review from yayi-google Feb 19, 2021
@yirutang yirutang marked this pull request as ready for review Feb 19, 2021
@stephaniewang526 stephaniewang526 added the kokoro:force-run label Feb 19, 2021
@yirutang
Copy link
Contributor Author

@yirutang yirutang commented Feb 22, 2021

Having issues running Yi's stress test.

@stephaniewang526 stephaniewang526 added kokoro:force-run and removed kokoro:force-run labels Feb 22, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Feb 22, 2021
@yayi-google
Copy link
Contributor

@yayi-google yayi-google commented Feb 22, 2021

@yirutang yirutang requested a review from Feb 23, 2021
@yirutang yirutang requested a review from as a code owner Feb 23, 2021
@generated-files-bot
Copy link

@generated-files-bot generated-files-bot bot commented Feb 23, 2021

Warning: This pull request is touching the following templated files:

  • samples/install-without-bom/pom.xml
  • samples/snippets/pom.xml

@snippet-bot
Copy link

@snippet-bot snippet-bot bot commented Feb 23, 2021

Here is the summary of changes.

You are about to add 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@yirutang yirutang force-pushed the fixfix branch 2 times, most recently from ec96e06 to dc5ffde Compare Feb 23, 2021
@codecov
Copy link

@codecov codecov bot commented Feb 23, 2021

Codecov Report

Merging #854 (37db867) into master (7c01f45) will increase coverage by 0.36%.
The diff coverage is 83.13%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #854      +/-   ##
============================================
+ Coverage     80.28%   80.65%   +0.36%     
- Complexity      961     1016      +55     
============================================
  Files            76       76              
  Lines          5311     5552     +241     
  Branches        405      432      +27     
============================================
+ Hits           4264     4478     +214     
- Misses          875      900      +25     
- Partials        172      174       +2     
Impacted Files Coverage Δ Complexity Δ
...e/cloud/bigquery/storage/v1beta2/StreamWriter.java 79.20% <64.93%> (-3.54%) 31.00 <6.00> (-1.00)
...cloud/bigquery/storage/v1beta2/StreamWriterV2.java 96.13% <97.84%> (+1.17%) 35.00 <15.00> (+12.00)
.../google/cloud/bigquery/storage/v1beta2/Waiter.java 56.04% <100.00%> (+2.11%) 12.00 <0.00> (ø)
...d/bigquery/storage/v1beta2/JsonToProtoMessage.java 96.85% <0.00%> (-0.77%) 44.00% <0.00%> (ø%)
.../bigquery/storage/v1alpha2/JsonToProtoMessage.java 96.96% <0.00%> (-0.75%) 47.00% <0.00%> (ø%)
.../cloud/bigquery/storage/v1alpha2/StreamWriter.java 84.31% <0.00%> (-0.09%) 37.00% <0.00%> (ø%)
...torage/v1beta2/BQTableSchemaToProtoDescriptor.java 98.48% <0.00%> (+0.07%) 8.00% <0.00%> (ø%)
...orage/v1alpha2/BQTableSchemaToProtoDescriptor.java 98.48% <0.00%> (+0.07%) 8.00% <0.00%> (ø%)
... and 26 more

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 7c01f45...31af7be. Read the comment docs.

@yirutang
Copy link
Contributor Author

@yirutang yirutang commented Feb 23, 2021

Having issues running Yi's stress test.

@yirutang yirutang closed this Feb 23, 2021
@yirutang yirutang reopened this Feb 23, 2021
@stephaniewang526 stephaniewang526 added the status: investigating label Feb 24, 2021
@stephaniewang526 stephaniewang526 removed the status: investigating label Feb 26, 2021
@stephaniewang526
Copy link
Member

@stephaniewang526 stephaniewang526 commented Feb 26, 2021

This test keeps failing:

StreamWriterTest.testFlowControlBehaviorBlock:574 Call got exception: java.lang.IllegalStateException: Cannot append on a shut-down writer.

Full trace

@yirutang
Copy link
Contributor Author

@yirutang yirutang commented Feb 26, 2021

@stephaniewang526 stephaniewang526 merged commit 0870797 into googleapis:master Mar 2, 2021
15 of 16 checks passed
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 4, 2021
🤖 I have created a release \*beep\* \*boop\*
---
## [1.14.0](https://www.github.com/googleapis/java-bigquerystorage/compare/v1.13.0...v1.14.0) (2021-03-04)


### Features

* update StreamWriterV2 to support trace id ([#895](https://www.github.com/googleapis/java-bigquerystorage/issues/895)) ([2e49ce8](https://www.github.com/googleapis/java-bigquerystorage/commit/2e49ce8c79cb059840c3307898ba16980f6892fa))


### Bug Fixes

* add schema update back to json writer ([#905](https://www.github.com/googleapis/java-bigquerystorage/issues/905)) ([a2adbf8](https://www.github.com/googleapis/java-bigquerystorage/commit/a2adbf80753161cbddd23d5a7db75e9250db58fa))
* Add unit test for concurrent issues we worried about, and fix some locking issues ([#854](https://www.github.com/googleapis/java-bigquerystorage/issues/854)) ([0870797](https://www.github.com/googleapis/java-bigquerystorage/commit/087079728195e20f93701e8d5e1e59ba29a7d21b))
* test failure testAppendWhileShutdownSuccess ([#904](https://www.github.com/googleapis/java-bigquerystorage/issues/904)) ([b80183e](https://www.github.com/googleapis/java-bigquerystorage/commit/b80183ea23c8b78611a42d22d8c62a4ba4904a80))
* testAppendWhileShutdownSuccess race ([#907](https://www.github.com/googleapis/java-bigquerystorage/issues/907)) ([d39443d](https://www.github.com/googleapis/java-bigquerystorage/commit/d39443d51d2625e4b3aee59d1e593229e9e449d3))


### Dependencies

* update dependency com.google.cloud:google-cloud-bigquery to v1.127.6 ([#909](https://www.github.com/googleapis/java-bigquerystorage/issues/909)) ([505938b](https://www.github.com/googleapis/java-bigquerystorage/commit/505938bcba5a4a7af9e618572bbc41f365702f47))
---


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 fixfix branch Apr 30, 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

4 participants