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

Pubsub: Fix subscriber.stopAsync().awaitTerminated() does not destroy respective threads. #5495

Closed
wants to merge 8 commits into from

Conversation

pmakani
Copy link

@pmakani pmakani commented Jun 14, 2019

Fixes #5227, #4211

@pmakani pmakani requested a review from a team as a code owner June 14, 2019 14:18
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 14, 2019
@codecov
Copy link

codecov bot commented Jun 14, 2019

Codecov Report

Merging #5495 into master will decrease coverage by <.01%.
The diff coverage is 60%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5495      +/-   ##
============================================
- Coverage     50.82%   50.82%   -0.01%     
+ Complexity    24189    24175      -14     
============================================
  Files          2271     2271              
  Lines        229912   229918       +6     
  Branches      25002    25007       +5     
============================================
+ Hits         116845   116848       +3     
- Misses       104435   104438       +3     
  Partials       8632     8632
Impacted Files Coverage Δ Complexity Δ
...in/java/com/google/cloud/pubsub/v1/Subscriber.java 77.77% <100%> (+0.24%) 17 <0> (ø) ⬇️
.../com/google/cloud/pubsub/v1/MessageDispatcher.java 84.89% <50%> (-0.97%) 23 <0> (ø)
...cloud/pubsub/v1/StreamingSubscriberConnection.java 64% <50%> (-0.76%) 9 <2> (ø)
...oogle/cloud/pubsub/v1/stub/GrpcSubscriberStub.java 94.48% <0%> (+0.25%) 23% <0%> (+1%) ⬆️

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 a560bb5...579bc89. Read the comment docs.

@sduskis
Copy link
Contributor

sduskis commented Jun 14, 2019

@pmakani, thanks for the PR. I spoke at length with @ajaaym about this. I think we will have to defer to @kamalaboulhosn and team about how to proceed with this. There are a number of things to consider as part of the shutdown process.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 21, 2019
@sduskis
Copy link
Contributor

sduskis commented Jun 28, 2019

@pmakani, the Pub/Sub team will work on this.

@sduskis sduskis closed this Jun 28, 2019
@pmakani pmakani reopened this Aug 21, 2019
@pmakani
Copy link
Author

pmakani commented Aug 21, 2019

@hannahrogers-google PTAL.

@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Aug 21, 2019
@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 23, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 23, 2019
@hannahrogers-google
Copy link

@pmakani, thanks for creating this PR. I have discussed this issue with other members of the Pub/Sub team to determine the best solution.

We would like to close the subStub, as you do in this PR. However, as part of the shutdown process, we would like make sure we process all outstanding acks/nacks. We would only like to close the stub once all related rpcs have completed.

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.

PubSub: subscriber.stopAsync().awaitTerminated() does not destroy respective threads
6 participants