Skip to content

fix: shutdown grpc stubs properly when a subscriber is stopped #74

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.

Sign up for GitHub

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

Jump to bottom
Merged
kamalaboulhosn merged 17 commits into from
Jan 31, 2020
Merged

fix: shutdown grpc stubs properly when a subscriber is stopped #74

kamalaboulhosn merged 17 commits into from
Jan 31, 2020

Conversation

@hannahrogers-google
Copy link
Contributor

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

Fixes #22

@hannahrogers-google
Modifying Publish example in README to match other examples given, and
19dd129 
fix issue #6784
@hannahrogers-google
fix: Modifying Publish example in README to match other examples, and
4158529 
fix Issue googleapis#11
@hannahrogers-google
Merge branch 'master' of github.com:hannahrogers-google/java-pubsub
7d704f0
@hannahrogers-google
Merge branch 'master' of https://github.com/googleapis/java-pubsub
9e0ebc6
@hannahrogers-google
feat: Adding support for DLQs
01d41b7 
Adding delivery attempt count to PubsubMessages as a message attribute,
and creating helper function to allow users to get the count without
knowing implementation details.
@hannahrogers-google
Fix formatting
ed3b1ee
@hannahrogers-google
fix: making changes requested in pull request
72f7996
@hannahrogers-google
Merge branch 'master' of https://github.com/googleapis/java-pubsub
389cb86
@hannahrogers-google
Merge branch 'master' of https://github.com/googleapis/java-pubsub
c9e4bd2
@hannahrogers-google
fix: creating fix to not populate delivery attempt attribute when dead
3738ec8 
lettering is not enabled
@hannahrogers-google
Merge branch 'master' of https://github.com/googleapis/java-pubsub
4ce1d3b
@hannahrogers-google
Adding unit test for case in which a received message has no delivery…
aecd4ca 
… attempt
@hannahrogers-google
Making MessageWaiter class more generic to also be used for outstanding
77fa3b3 
ack operations
@hannahrogers-google
Merge branch 'master' of https://github.com/googleapis/java-pubsub
b87023c
@hannahrogers-google
Merge branch 'master' into fix-subscriber-stop
7086b3c
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 30, 2020
@hannahrogers-google hannahrogers-google added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 30, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 30, 2020
@codecov
Copy link

codecov bot commented Jan 30, 2020 •
edited
Loading

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 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
@yoshi-automation yoshi-automation mentioned this pull request Feb 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kamalaboulhosn kamalaboulhosn kamalaboulhosn approved these changes

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@hannahrogers-google @kamalaboulhosn @googlebot @yoshi-kokoro