Skip to content

Conversation

@mdirkse
Copy link

@mdirkse mdirkse commented Sep 9, 2019

Completes fix of #5227.

We found that the PubSub Subscriber can go into a thread creation loop when it fails on startup, eventually causing system-wide thread starvation. This PR fixes that problem by making sure that all background resources are also shut down on failure (instead of just when stopped).

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 9, 2019
Copy link

@hannahrogers-google hannahrogers-google left a comment

Choose a reason for hiding this comment

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

@mdirkse, thanks for your PR! Looks good - just a small change requested for the unit test

@hannahrogers-google
Copy link

@mdirkse, did you want to take a look at the requested changes so that we can merge this PR?

@mdirkse
Copy link
Author

mdirkse commented Oct 15, 2019

Yes! Sorry, will do so today.

@mdirkse mdirkse force-pushed the master branch 2 times, most recently from 4f40b49 to 70a0bc6 Compare October 17, 2019 19:45
Copy link

@hannahrogers-google hannahrogers-google left a comment

Choose a reason for hiding this comment

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

Thanks @mdirkse!

@hongalex hongalex added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 31, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 31, 2019
@codecov
Copy link

codecov bot commented Nov 7, 2019

Codecov Report

Merging #6238 into master will increase coverage by 0.36%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6238      +/-   ##
============================================
+ Coverage        46%   46.36%   +0.36%     
- Complexity    26517    28037    +1520     
============================================
  Files          2614     2614              
  Lines        282790   288197    +5407     
  Branches      33598    33789     +191     
============================================
+ Hits         130084   133614    +3530     
- Misses       143430   144333     +903     
- Partials       9276    10250     +974
Impacted Files Coverage Δ Complexity Δ
...in/java/com/google/cloud/pubsub/v1/Subscriber.java 79.32% <100%> (+1.61%) 20 <2> (+3) ⬆️
...va/com/google/cloud/compute/v1/InstanceClient.java 55.09% <0%> (-7.79%) 147% <0%> (+36%)
...onitoring/v3/NotificationChannelServiceClient.java 64.49% <0%> (-7.13%) 43% <0%> (+10%)
...ava/com/google/cloud/compute/v1/ProjectClient.java 57.07% <0%> (-6.99%) 55% <0%> (+13%)
.../com/google/cloud/compute/v1/TargetPoolClient.java 57.73% <0%> (-6.98%) 47% <0%> (+11%)
...va/com/google/cloud/compute/v1/SnapshotClient.java 55.73% <0%> (-6.88%) 31% <0%> (+7%)
...oogle/cloud/kms/v1/KeyManagementServiceClient.java 62.88% <0%> (-6.84%) 103% <0%> (+25%)
...com/google/cloud/compute/v1/ReservationClient.java 59.14% <0%> (-6.83%) 39% <0%> (+9%)
...a/com/google/cloud/compute/v1/NodeGroupClient.java 60.36% <0%> (-6.82%) 51% <0%> (+12%)
.../com/google/cloud/compute/v1/SubnetworkClient.java 60.36% <0%> (-6.82%) 51% <0%> (+12%)
... and 640 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 fe11ffa...adf0d35. Read the comment docs.

@mdirkse
Copy link
Author

mdirkse commented Nov 7, 2019

Fixed the formatting error.

@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2019
@hannahrogers-google hannahrogers-google merged commit dd5bb61 into googleapis:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants