Skip to content

revert setting of PayloadConfig in ServerConfig of non-generic server#9612

Merged
apolcyn merged 3 commits intogrpc:masterfrom
apolcyn:fix_csharp_benchmark_server_startups
Feb 9, 2017
Merged

revert setting of PayloadConfig in ServerConfig of non-generic server#9612
apolcyn merged 3 commits intogrpc:masterfrom
apolcyn:fix_csharp_benchmark_server_startups

Conversation

@apolcyn
Copy link
Copy Markdown
Contributor

@apolcyn apolcyn commented Feb 7, 2017

fixes #9587

changes to scenario config set PayloadConfig for AsyncServer (protobuf config) - previous null check now failing.

@apolcyn
Copy link
Copy Markdown
Contributor Author

apolcyn commented Feb 7, 2017

c# performance tests passing here

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.

why does PayloadConfig needs to be set? For AsyncServer (= non-generic), the request should contain the payload config to use for the response, so something is wrong here.
Which PR have populated payload_config?

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.

I think the config change that added PayloadConfig to ServerConfig was 412fa2a.

cc @ctiller

I'm guessing it's ignored in other languages, as it used to not be set.

@jtattermusch
Copy link
Copy Markdown
Contributor

So this PR tracks down the rootcause correctly, thanks for investigating this @apolcyn.

Nevertheless, the way to fix this is to avoid setting server.payload_config - it's incorrect to do that for non-generic servers, for the reasons I described in #9392 (which seems to be the offending PR).

@jtattermusch
Copy link
Copy Markdown
Contributor

To set response size for non-generic servers,

int32 resp_size = 2;
should be set in client's payload_config.

@apolcyn apolcyn changed the title remove fatal null check in c# protobuf QPS server revert setting of PayloadConfig in ServerConfig of non-generic server Feb 8, 2017
@apolcyn
Copy link
Copy Markdown
Contributor Author

apolcyn commented Feb 8, 2017

@ctiller this reverts part of the change in 412fa2a. Is this ok?

Copy link
Copy Markdown
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM

@apolcyn
Copy link
Copy Markdown
Contributor Author

apolcyn commented Feb 9, 2017

re-ran generate projects

jenkins: test this please

@apolcyn apolcyn force-pushed the fix_csharp_benchmark_server_startups branch from 187beef to 923f8a3 Compare February 9, 2017 07:54
@apolcyn
Copy link
Copy Markdown
Contributor Author

apolcyn commented Feb 9, 2017

sanity still failing, rebased - not sure if this PR yet.

jenkins: test this please

@apolcyn apolcyn force-pushed the fix_csharp_benchmark_server_startups branch from 923f8a3 to e0aed71 Compare February 9, 2017 19:23
@apolcyn
Copy link
Copy Markdown
Contributor Author

apolcyn commented Feb 9, 2017

sanity should be fixed now.

jenkins: test this please

@apolcyn
Copy link
Copy Markdown
Contributor Author

apolcyn commented Feb 9, 2017

@apolcyn apolcyn merged commit a68089b into grpc:master Feb 9, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 2019
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request Feb 12, 2021
To better support config dump, deprecated field detection and other debug, it's helpful to leave a type name breadcrumb and be able to synthesize a Protobuf::Message that corresponds to what was delivered on the wire.

While working on this PR, it became apparent that config dump is broken post v3alpha, since a single config dump might have both v2 and v3 Listeners, etc. The only way to resolve this generically is to make the inner resources in config dump Any. This is a breaking API change, but these are v2alpha/v3alpha at this point, so allowed.

Risk level: Low
Testing: new version converter unit test, config dump tests now verify that the correct versioned inner resource is returned.

Fixes grpc#9612

Signed-off-by: Harvey Tuch <[email protected]>

Mirrored from https://github.com/envoyproxy/envoy @ 233838a39fb8310ea145e7d758d428d5a37b0306
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

csharp benchmark failures

4 participants