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.

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

feat: add a close() method to PubSub, and a flush() method to Topic/Publisher #916

Merged
merged 6 commits into from Mar 26, 2020
Merged

feat: add a close() method to PubSub, and a flush() method to Topic/Publisher #916

merged 6 commits into from Mar 26, 2020

Conversation

feywind
Copy link
Collaborator

@feywind feywind commented Mar 18, 2020

Fixes: #817

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 18, 2020
@feywind
Copy link
Collaborator Author

feywind commented Mar 19, 2020

Looks like there's an issue in checkout@v1, and we hadn't updated all of the CI actions to v2.
actions/checkout#23

bcoe
bcoe approved these changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, not seeing any possums.

.github/workflows/ci.yaml Show resolved Hide resolved
src/publisher/index.ts
);
const allPublishes = Promise.all(publishes);

allPublishes
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to use async/await here? I'm at the point where I pretty much always favor it these days.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I about 350% agree... unfortunately the current code is mostly callback-based, and a few event-based bits, so pulling the async/await far up the stack was looking difficult. I'd really like to go back and do a pass of just converting everything to straightforward async/await/Promises, but I don't want to do that as part of this.

src/pubsub.ts
const definedCallback = callback || (() => {});
if (this.isOpen) {
this.closeAllClients_()
.then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment, as above. I can understand the argument for keeping the codebase internally consistent, if that's the thinking.

src/pubsub.ts Outdated Show resolved Hide resolved
test/pubsub.ts Show resolved Hide resolved
@feywind feywind mentioned this pull request Mar 25, 2020
Closed
@feywind feywind added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2020
@feywind
Copy link
Collaborator Author

feywind commented Mar 25, 2020

I added a note about the potentially flaky test, re-running Kokoro for now.

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 25, 2020
@feywind feywind merged commit 4097995 into googleapis:master Mar 26, 2020
@feywind feywind deleted the close-method-gh817 branch March 26, 2020 18:03
gcf-merge-on-green bot pushed a commit that referenced this pull request Mar 30, 2020
@release-please
chore: release 1.7.0 (#933)
b96eacf 
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.7.0](https://www.github.com/googleapis/nodejs-pubsub/compare/v1.6.0...v1.7.0) (2020-03-29)


### Features

* add a close() method to PubSub, and a flush() method to Topic/Publisher ([#916](https://www.github.com/googleapis/nodejs-pubsub/issues/916)) ([4097995](https://www.github.com/googleapis/nodejs-pubsub/commit/4097995a85a8ca3fb73c2c2a8cb0649cdd4274be))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
@mad-it mad-it mentioned this pull request Mar 31, 2020
Closed
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
@yoshi-automation
feat: add a close() method to PubSub, and a flush() method to Topic/P…
a56e5c4 
…ublisher (#916)

4097995
commit 4097995
Author: Megan Potter <57276408+feywind@users.noreply.github.com>
Date:   Thu Mar 26 11:03:26 2020 -0700

    feat: add a close() method to PubSub, and a flush() method to Topic/Publisher (#916)

    * docs: fix a typo in a comment

    * feat: allow manually closing the server connections in the PubSub object

    * feat: add flush() method for Topic objects

    * tests: add tests for new flush() and close() methods

    * build: update github checkout action to v2 to fix spurious retry errors

    * fix: set isOpen to false before trying to close it so that all usage will stop
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
@yoshi-automation
chore: release 1.7.0 (#933)
b58cc90 
b96eacf
commit b96eacf
Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Date:   Mon Mar 30 21:46:18 2020 +0000

    chore: release 1.7.0 (#933)

    🤖 I have created a release \*beep\* \*boop\*
    ---
    ## [1.7.0](https://www.github.com/googleapis/nodejs-pubsub/compare/v1.6.0...v1.7.0) (2020-03-29)

    ### Features

    * add a close() method to PubSub, and a flush() method to Topic/Publisher ([#916](https://www.github.com/googleapis/nodejs-pubsub/issues/916)) ([4097995](https://www.github.com/googleapis/nodejs-pubsub/commit/4097995a85a8ca3fb73c2c2a8cb0649cdd4274be))
    ---

    This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe 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.

Publisher Client close() method
4 participants
@feywind @bcoe @googlebot @yoshi-kokoro