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

[pack] adding AspNetCore Grpc implementation #7543

Merged
merged 5 commits into from
Sep 20, 2021
Merged

Conversation

brettsam
Copy link
Member

@brettsam brettsam commented Jul 14, 2021

This is a revitalization of #6842, hiding the implementation behind a feature flag.

Update -- this is no longer hidden behind a feature flag, but you can use a feature flag to disable and go back to the old Grpc server implementation.

@brettsam brettsam force-pushed the brettsam/update-grpc branch from ed5833e to 172f9a2 Compare September 15, 2021 19:46
@brettsam
Copy link
Member Author

Capturing the deps.json failure here so I can comment:

IMPORTANT: The dependencies in WebHost have changed and MUST be reviewed before proceeding. Please follow up with brettsam, fabiocav or mathewc for approval.

Previous file: D:\a\1\s\test\WebJobs.Script.Tests\bin\Debug\netcoreapp3.1\Microsoft.Azure.WebJobs.Script.WebHost.deps.json
New file:      D:\a\1\s\src\WebJobs.Script.WebHost\bin\Debug\netcoreapp3.1\Microsoft.Azure.WebJobs.Script.WebHost.deps.json

Changed:
- Google.Protobuf.dll: 3.11.4.0/3.11.4.0 -> 3.13.0.0/3.13.0.0

Removed:
- Grpc.Core.Api.dll: 2.0.0.0/2.27.0.0
- Grpc.Core.dll: 2.0.0.0/2.27.0.0

Added:
- Grpc.AspNetCore.Server.ClientFactory.dll: 2.0.0.0/2.32.0.0
- Grpc.AspNetCore.Server.dll: 2.0.0.0/2.32.0.0
- Grpc.Core.Api.dll: 2.0.0.0/2.32.0.0
- Grpc.Core.dll: 2.0.0.0/2.32.0.0
- Grpc.Net.Client.dll: 2.0.0.0/2.32.0.0
- Grpc.Net.ClientFactory.dll: 2.0.0.0/2.32.0.0
- Grpc.Net.Common.dll: 2.0.0.0/2.32.0.0
- Microsoft.Extensions.Configuration.Abstractions.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Configuration.Binder.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Configuration.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.DependencyInjection.Abstractions.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.DependencyInjection.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Logging.Abstractions.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Logging.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Options.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Primitives.dll: 3.1.9.0/3.100.920.47302

@brettsam
Copy link
Member Author

First, the easy ones:

These are all used already because we run on the 3.1 framework. I confirmed in a running app already.

- Microsoft.Extensions.Configuration.Abstractions.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Configuration.Binder.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Configuration.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.DependencyInjection.Abstractions.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.DependencyInjection.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Logging.Abstractions.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Logging.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Options.dll: 3.1.9.0/3.100.920.47302
- Microsoft.Extensions.Primitives.dll: 3.1.9.0/3.100.920.47302

These are minor version updates (I'm trying to figure out why some of these are listed as Removed/Added rather than Changed):

Changed:
- Google.Protobuf.dll: 3.11.4.0/3.11.4.0 -> 3.13.0.0/3.13.0.0

Removed:
- Grpc.Core.Api.dll: 2.0.0.0/2.27.0.0
- Grpc.Core.dll: 2.0.0.0/2.27.0.0

Added:
...
- Grpc.Core.Api.dll: 2.0.0.0/2.32.0.0
- Grpc.Core.dll: 2.0.0.0/2.32.0.0

I think we should explicitly list these as Private just in case anyone happens to be using them in prod:

Added:
- Grpc.AspNetCore.Server.ClientFactory.dll: 2.0.0.0/2.32.0.0
- Grpc.AspNetCore.Server.dll: 2.0.0.0/2.32.0.0
...
- Grpc.Net.Client.dll: 2.0.0.0/2.32.0.0
- Grpc.Net.ClientFactory.dll: 2.0.0.0/2.32.0.0
- Grpc.Net.Common.dll: 2.0.0.0/2.32.0.0

@brettsam brettsam requested a review from mathewc as a code owner September 16, 2021 21:59
@pragnagopa
Copy link
Member

Tagging @Francisco-Gamino as FYI - need to keep a close on E2E integration test results after this PR is merged - to ensure we do not see any issues across language workers

Copy link
Member

@pragnagopa pragnagopa left a comment

Choose a reason for hiding this comment

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

💯

cc @balag0 - fyi

@pragnagopa
Copy link
Member

tagging @vrdmr - for python language worker , @amamounelsayed - for language worker , @alrod - for node language worker , @AnatoliB / @Francisco-Gamino - powershell language worker

FYI - please keep a close eye on the logs as this commit starts rolling out

@Francisco-Gamino
Copy link
Contributor

Tagging @Francisco-Gamino as FYI - need to keep a close on E2E integration test results after this PR is merged - to ensure we do not see any issues across language workers

@pragnagopa -- Ack. Will do.

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.

3 participants