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: shutdown grpc stubs properly when a subscriber is stopped #74

Merged

Conversation

hannahrogers-google
Copy link
Contributor

@hannahrogers-google hannahrogers-google commented Jan 30, 2020

Fixes #22

@googlebot googlebot added the cla: yes label Jan 30, 2020
@hannahrogers-google hannahrogers-google added the kokoro:force-run label Jan 30, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run label Jan 30, 2020
@codecov
Copy link

@codecov codecov bot commented Jan 30, 2020

Codecov Report

Merging #74 into master will decrease coverage by <.01%.
The diff coverage is 82.6%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #74      +/-   ##
============================================
- Coverage     78.94%   78.93%   -0.01%     
- Complexity      292      293       +1     
============================================
  Files            21       21              
  Lines          2588     2597       +9     
  Branches        129      129              
============================================
+ Hits           2043     2050       +7     
- Misses          490      492       +2     
  Partials         55       55
Impacted Files Coverage Δ Complexity Δ
.../com/google/cloud/pubsub/v1/MessageDispatcher.java 85.64% <100%> (ø) 25 <5> (ø) ⬇️
...c/main/java/com/google/cloud/pubsub/v1/Waiter.java 76.47% <100%> (ø) 6 <4> (?)
...in/java/com/google/cloud/pubsub/v1/Subscriber.java 79.67% <100%> (+0.11%) 22 <0> (ø) ⬇️
...ain/java/com/google/cloud/pubsub/v1/Publisher.java 89.07% <100%> (ø) 43 <0> (ø) ⬇️
...cloud/pubsub/v1/StreamingSubscriberConnection.java 63.84% <50%> (-0.91%) 9 <0> (ø)
...oogle/cloud/pubsub/v1/stub/GrpcSubscriberStub.java 94.23% <0%> (+0.54%) 22% <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 aecd4ca...dc55fc1. Read the comment docs.

@hannahrogers-google
Copy link
Contributor Author

@hannahrogers-google hannahrogers-google commented Jan 31, 2020

This PR modifies the code to do the following:

  1. Makes the MessageWaiter class more generic, so it can be used for all types of pending actions
  2. Uses the Waiter class to track outstanding ack/modAck operations that have not completed
  3. Waits for all outstanding ack operations to complete before shutting down a StreamingSubscriberConnection
  4. Shuts down the GrpcSubscriberStub when a subscriber has been stopped.

This PR should fix issue #22 and issue #28. Using the steps to reproduce the issue (provided in #28), I was able to confirm the problem. With this fix, I confirmed that the issue is no longer present.

@kamalaboulhosn kamalaboulhosn merged commit 9bcc433 into googleapis:master Jan 31, 2020
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants