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

GH-4155 - Increase gRPC message size #4455

Merged
merged 19 commits into from
Apr 25, 2020
Merged

GH-4155 - Increase gRPC message size #4455

merged 19 commits into from
Apr 25, 2020

Conversation

komalali
Copy link
Member

Fixes: #4155

Related: pulumi/pulumi-policy#239

Comments:

  • I tried to minimize the number of individual places where that 1024 * 1024 * 400 number is referenced by creating a util function, but that only helps for the Go code. Is there a preferred way of adding such a widespread setting?
  • Is the dotnet code adequate? Haven't ever worked in that lang before.
  • For grpc: received message larger than max #4155 to truly be fixed, providers will need to be regenerated with pulumi-terraform-bridge that pulls this updated code and the related pulumi-policy PR must be merged.

@komalali komalali self-assigned this Apr 20, 2020
@komalali komalali mentioned this pull request Apr 20, 2020
Merged
@komalali komalali marked this pull request as ready for review April 20, 2020 23:44
lukehoban
lukehoban reviewed Apr 20, 2020
View reviewed changes
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

Is there a way we could add some test coverage of this? A test in each language that sends a very large string across the registerResource RPC call?

sdk/dotnet/Pulumi/Deployment/GrpcEngine.cs Show resolved Hide resolved
sdk/go/pulumi/context.go Outdated Show resolved Hide resolved
sdk/nodejs/runtime/settings.ts Outdated Show resolved Hide resolved
@komalali komalali force-pushed the komalali/increase-grpc-message-size branch from bfa71ed to 52861a6 Compare April 21, 2020 18:31
lukehoban
lukehoban reviewed Apr 21, 2020
View reviewed changes
sdk/nodejs/tests/runtime/langhost/run.spec.ts
},
};
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Do these tests actually go through the RPC layer? I don't recall off the top of my head - but I feel like they do not? cc @pgavlin

One place we could add tests to be sure they hit the RPC is in /test/integration. Those will run the full "normal" CLI/Engine/LangHost setup.

Copy link
Member

Choose a reason for hiding this comment

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

BTW - same likely applies to other languages.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lukehoban yes they do. I made sure by checking that they fail on master with the same resource exhausted error. I can definitely add integration tests too but I am certain that these tests go through the langhost <-> engine RPC layer.

@komalali komalali force-pushed the komalali/increase-grpc-message-size branch from 71dbd0e to 9dff162 Compare April 23, 2020 18:10
@komalali komalali force-pushed the komalali/increase-grpc-message-size branch from 9dff162 to 281dc8c Compare April 23, 2020 18:18
@komalali komalali force-pushed the komalali/increase-grpc-message-size branch from 281dc8c to 0139b8f Compare April 23, 2020 18:21
@komalali komalali force-pushed the komalali/increase-grpc-message-size branch from 9e75c51 to 2776a80 Compare April 24, 2020 01:05
@komalali
Copy link
Member Author

Okay! I added in the integration tests here, added the skipped Node.js test in Windows to the list in #3811 and I think we're good to go here.

@komalali komalali requested a review from lukehoban April 24, 2020 22:07
lukehoban
lukehoban approved these changes Apr 25, 2020
View reviewed changes
Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

Looks great! 🎉

@komalali komalali merged commit 21ccfb8 into master Apr 25, 2020
@pulumi-bot pulumi-bot deleted the komalali/increase-grpc-message-size branch April 25, 2020 02:02
abhinav pushed a commit to pulumi/pulumi-dotnet that referenced this pull request Jan 11, 2023
@komalali
Merge pull request pulumi/pulumi#4455 from pulumi/komalali/increase-g…
b647d2a 
…rpc-message-size

GH-4155 - Increase gRPC message size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukehoban lukehoban lukehoban approved these changes

@stack72 stack72 Awaiting requested review from stack72

@pgavlin pgavlin Awaiting requested review from pgavlin

Assignees

@komalali komalali

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

grpc: received message larger than max
2 participants
@komalali @lukehoban