Skip to content
This repository was archived by the owner on Dec 16, 2020. It is now read-only.

Fix Wasm gRPC initial metadata callback#417

Closed
bianpengyuan wants to merge 5 commits intoenvoyproxy:masterfrom
bianpengyuan:wasm-create-initial-metadata
Closed

Fix Wasm gRPC initial metadata callback#417
bianpengyuan wants to merge 5 commits intoenvoyproxy:masterfrom
bianpengyuan:wasm-create-initial-metadata

Conversation

@bianpengyuan
Copy link
Copy Markdown
Contributor

#414

This change adds two member variables to gRPC handler, so that onCreateInitialMetadata could be triggered even the call token is not ready in the module.

cc @PiotrSikora

Signed-off-by: Pengyuan Bian bianpengyuan@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
@bianpengyuan bianpengyuan force-pushed the wasm-create-initial-metadata branch from fdbabfa to 1fed5c4 Compare February 19, 2020 22:35
Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
@PiotrSikora
Copy link
Copy Markdown
Contributor

The test failures are real, you need to update API, regenerate SDK and/or update test files.

Also, could you please add a test exercising gRPC callout (I thought we had one, so I'm not sure why it didn't catch this issue?), to make sure that the fix works?

Signed-off-by: Pengyuan Bian <bianpengyuan@google.com>
@bianpengyuan
Copy link
Copy Markdown
Contributor Author

Yeah, wasm gen make file misses root_id.wasm. Seems like gRPC test does not exercise initial metadata code path. Will add a case for it.

Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora left a comment

Choose a reason for hiding this comment

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

Marking as blocked, pending changes discussed in #414.

@bianpengyuan
Copy link
Copy Markdown
Contributor Author

Supersede by #461

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants