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.

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

Document missing options #1983

Closed
wants to merge 3 commits into from
Closed

Conversation

hermanbanken
Copy link

@hermanbanken hermanbanken commented Dec 7, 2021

  • grpc.max_connection_age_grace_ms
  • grpc.max_connection_age_ms

We were bitten by these missing options. One could argue that it is unclear how to best setup gRPC round robin load balancing in a Kubernetes cluster, but that is besides the point. We were using these options, switched from grpc to grpc-js and suddenly the load was only coming to a few instances, making the service 10x as slow. I'd argue this is missing from the "migration" list in the readme too: https://www.npmjs.com/package/@grpc/grpc-js.

@hermanbanken
Copy link
Author

I wrote this in the readme:

If you were using grpc.max_connection_age_grace_ms or grpc.max_connection_age_ms, please note that these options are not implemented. If you are using this for load balancing, you should migrate to proper client-side load balancing or service configs.

But I have to say, I do not know where to find proper client-side load balancing or service configs documentation myself... I'd like to add a link to some good documentation there.

@murgatroid99
Copy link
Member

I don't see any value in calling out specific options that are not implemented. All of the options not listed are not implemented; there is nothing special about the ones you added the note about.

Similarly, I don't think we should call out specific options in the README. We could instead have a general note referencing the list of supported options.

@hermanbanken
Copy link
Author

I don't see any value in calling out specific options that are not implemented. All of the options not listed are not implemented; there is nothing special about the ones you added the note about.

Similarly, I don't think we should call out specific options in the README. We could instead have a general note referencing the list of supported options.

I wholly disagree. If you are coming from the grpc library, and you read this:

Documentation specifically for the @grpc/grpc-js package is currently not available. However, documentation is available for the grpc package, and the two packages contain mostly the same interface. There are a few notable differences, however, and these differences are noted in the "Migrating from grpc" section below.

Then you expect everything to be supported. From that moment on, everything unsupported I expect to be listed explicitly.

The value in adding this is that for real production use cases (the ones with actual load) there ain't any new unexpected behaviour. This cost us easily 2 hours already (multiple people investigating this in parallel) and multiply that by everyone that is using these options and you have substantial value.

@murgatroid99
Copy link
Member

I think it would be reasonable to migrate the list of supported options from that linked document to the README. However, I don't think it's practical to maintain a list of every option the library doesn't support, and I think it would be misleading to specifically call out only some unsupported options.

josephharrington added a commit to josephharrington/grpc-node that referenced this pull request Dec 27, 2021
This copies the list of supported channel arguments from PACKAGE_COMPARISON.md into the readme, and also adds a link to the package comparison doc.

resolves grpc#1982, resolves grpc#1983
josephharrington added a commit to josephharrington/grpc-node that referenced this pull request Jan 4, 2022
This moves the list of supported channel arguments from PACKAGE_COMPARISON.md into the readme, and also adds a link to the package comparison doc.

resolves grpc#1982, resolves grpc#1983
josephharrington added a commit to josephharrington/grpc-node that referenced this pull request Jan 5, 2022
This moves the list of supported channel arguments from PACKAGE_COMPARISON.md into the readme, and also adds a link to the package comparison doc.

resolves grpc#1982, resolves grpc#1983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants