Skip to content
This repository was archived by the owner on Nov 18, 2025. It is now read-only.

fix: set default values for gRPC parameters#840

Merged
xiaozhenliu-gg5 merged 9 commits intomasterfrom
grpc-default
Jun 3, 2020
Merged

fix: set default values for gRPC parameters#840
xiaozhenliu-gg5 merged 9 commits intomasterfrom
grpc-default

Conversation

@xiaozhenliu-gg5
Copy link
Contributor

@xiaozhenliu-gg5 xiaozhenliu-gg5 commented Jun 2, 2020

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 2, 2020
@alexander-fenster
Copy link
Contributor

alexander-fenster commented Jun 2, 2020

It affects behavior, so it's not a chore but a fix. Other than that, LGTM.

@alexander-fenster alexander-fenster changed the title chore: set grpc default value fix: set default values for gRPC parameters Jun 2, 2020
@alexander-fenster
Copy link
Contributor

alexander-fenster commented Jun 2, 2020

KMS samples tests failed - it's interesting why they are affected. Does it reproduce locally?

@xiaozhenliu-gg5
Copy link
Contributor Author

xiaozhenliu-gg5 commented Jun 2, 2020 •
edited
Loading

It can be reproduced locally, it complains about, from the test log, it's because of the timeout. But not sure why this changes triggers this failure.

  1) Run system tests for some libraries
       kms
         should pass samples tests:
     Error: Command failed with exit code 1: npm run samples-test

@codecov
Copy link

codecov bot commented Jun 3, 2020 •
edited
Loading

Codecov Report

Merging #840 into master will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     googleapis/gax-nodejs#840      +/-   ##
==========================================
- Coverage   89.76%   89.71%   -0.06%     
==========================================
  Files          46       46              
  Lines        7443     7445       +2     
  Branches      562      562              
==========================================
- Hits         6681     6679       -2     
- Misses        759      763       +4     
  Partials        3        3
Impacted Files Coverage Δ
src/grpc.ts 93.76% <100.00%> (+0.03%) ⬆️
.mocharc.js 78.57% <0.00%> (-14.29%) ⬇️

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 c401b80...25616b1. Read the comment docs.

JustinBeckwith
JustinBeckwith reviewed Jun 3, 2020
View reviewed changes
test/system-test/test.clientlibs.ts Outdated
await preparePackage('nodejs-kms');
});
it('should pass samples tests', async () => {
it.only('should pass samples tests', async () => {
Copy link
Contributor

@JustinBeckwith JustinBeckwith Jun 3, 2020

Choose a reason for hiding this comment

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

Probably need to remove that :)

Copy link
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Jun 3, 2020

Choose a reason for hiding this comment

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

Thank you! I will keep this in mind. Currently one default value fails the KMS system-test, so I am testing the parameters, thought only running this test will save us some time. :) I will clean up after the test.

@xiaozhenliu-gg5
Copy link
Contributor Author

xiaozhenliu-gg5 commented Jun 3, 2020

Based on the documentation , the default value of 'grpc.initial_reconnect_backoff_ms' is 0.1s, which is also the value from kms/v1/cloudkms_grpc_service_config.json. And the "maxBackoff" is set to be "60s" Looking at the protocol if we set the initial larger to 5000, it might exceed the max backoff very quickly, which results in timeout.

alexander-fenster
alexander-fenster approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

Thank you, that makes sense now.

@xiaozhenliu-gg5 xiaozhenliu-gg5 merged commit f7ebfb6 into master Jun 3, 2020
@xiaozhenliu-gg5 xiaozhenliu-gg5 deleted the grpc-default branch June 3, 2020 18:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@JustinBeckwith JustinBeckwith JustinBeckwith approved these changes

@alexander-fenster alexander-fenster alexander-fenster approved these changes

Reviewers whose approvals may not affect merge requirements

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.

Set default gRPC parameters

4 participants

@xiaozhenliu-gg5 @alexander-fenster @JustinBeckwith @googlebot