Skip to content

fix: add close() in the wrapper#986

Merged
mutianf merged 7 commits intogoogleapis:mainfrom
mutianf:close
Jan 18, 2022
Merged

fix: add close() in the wrapper#986
mutianf merged 7 commits intogoogleapis:mainfrom
mutianf:close

Conversation

@mutianf
Copy link
Contributor

@mutianf mutianf commented Jan 14, 2022

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@mutianf mutianf requested a review from a team January 14, 2022 21:30
@mutianf mutianf requested a review from a team as a code owner January 14, 2022 21:30
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the googleapis/nodejs-bigtable API. label Jan 14, 2022
@mutianf mutianf changed the title fix: add close in the wrapper fix: add close() in the wrapper Jan 14, 2022
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

lgtm

@mutianf mutianf added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 14, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 14, 2022
Copy link
Contributor

@danieljbruce danieljbruce left a comment

Choose a reason for hiding this comment

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

Just one comment on a return value

Copy link
Contributor

@danieljbruce danieljbruce left a comment

Choose a reason for hiding this comment

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

Advice on making this a little more concise.

src/index.ts Outdated
* Terminate grpc channels and close all the clients.
*/
close(): Promise<void[]> {
const combined = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think to achieve what you want here we can condense the first ten lines of this function to const combined = Object.keys(this.api).map(clientType => this.api[clientType].close()).

Copy link
Contributor

Choose a reason for hiding this comment

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

If this isn't exactly right I can try to tweak it later or help with debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works, thank you!

/**
* Terminate grpc channels and close all the clients.
*/
close(): Promise<void[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

any good way to test this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can add a test to make sure request will fail after client is closed, I'll work on it next week!

Copy link
Contributor

@danieljbruce danieljbruce left a comment

Choose a reason for hiding this comment

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

Suggestions for end of close function

@mutianf mutianf added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 18, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 18, 2022
@mutianf mutianf added the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 18, 2022
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jan 18, 2022
@mutianf mutianf merged commit 839f429 into googleapis:main Jan 18, 2022
@mutianf mutianf deleted the close branch January 18, 2022 15:13
@mutianf mutianf mentioned this pull request Jan 18, 2022
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 18, 2022
🤖 I have created a release *beep* *boop*
---


## [3.9.0](v3.8.0...v3.9.0) (2022-01-18)


### Features

* add Autoscaling API ([#963](#963)) ([86d21e8](86d21e8))


### Bug Fixes

* add close() in the wrapper ([#986](#986)) ([839f429](839f429))

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

Labels

api: bigtable Issues related to the googleapis/nodejs-bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants