Skip to content
This repository was archived by the owner on Mar 26, 2026. It is now read-only.

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
Copy Markdown
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 review from a team 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
Copy Markdown
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
Copy Markdown
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

Comment thread src/index.ts Outdated
Copy link
Copy Markdown
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.

Comment thread src/index.ts Outdated
* Terminate grpc channels and close all the clients.
*/
close(): Promise<void[]> {
const combined = [];
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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!

Comment thread src/index.ts
/**
* Terminate grpc channels and close all the clients.
*/
close(): Promise<void[]> {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread src/index.ts Outdated
@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).
GautamSharda pushed a commit to googleapis/google-cloud-node that referenced this pull request Mar 23, 2026
GautamSharda pushed a commit to googleapis/google-cloud-node that referenced this pull request Mar 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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