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

Add gRPC support for WASM #55

Merged
merged 14 commits into from
May 8, 2019
Merged

Add gRPC support for WASM #55

merged 14 commits into from
May 8, 2019

Conversation

jplevyak
Copy link

Add gRPC support for WASM

@istio-testing
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jplevyak
Copy link
Author

Let's see if that fix changes the behavior.

bianpengyuan
bianpengyuan reviewed May 1, 2019
View reviewed changes
Copy link

@bianpengyuan bianpengyuan left a comment

Choose a reason for hiding this comment

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

/lgtm

@bianpengyuan
Copy link

@jplevyak should we consider comment out throwException in _pthread_mutex_destroyHandler by default?

@jplevyak
Copy link
Author

jplevyak commented May 1, 2019

Yes, we should remove that as it is harmless.

PiotrSikora
PiotrSikora reviewed May 6, 2019
View reviewed changes
api/wasm/cpp/proxy_wasm_intrinsics_lite.proto Show resolved Hide resolved
api/wasm/cpp/proxy_wasm_intrinsics.h Show resolved Hide resolved
api/wasm/cpp/proxy_wasm_intrinsics.h Show resolved Hide resolved
api/wasm/cpp/proxy_wasm_intrinsics.h Show resolved Hide resolved
source/extensions/common/wasm/wasm.cc Outdated Show resolved Hide resolved
api/wasm/cpp/proxy_wasm_intrinsics.h Show resolved Hide resolved
api/wasm/cpp/proxy_wasm_intrinsics.h Show resolved Hide resolved
api/wasm/cpp/proxy_wasm_intrinsics.h Show resolved Hide resolved
api/wasm/cpp/proxy_wasm_intrinsics.h Show resolved Hide resolved
tools/check_format.py Outdated
@@ -16,7 +16,8 @@

EXCLUDED_PREFIXES = ("./generated/", "./thirdparty/", "./build", "./.git/", "./bazel-", "./.cache",
"./source/extensions/extensions_build_config.bzl",
"./tools/testdata/check_format/", "./tools/pyformat/", "./api/wasm/")
"./tools/testdata/check_format/", "./tools/pyformat/", "./api/wasm/",
"./test/extensions/filters/http/wasm/")
Copy link

Choose a reason for hiding this comment

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

Why do we need this? I don't see anything obvious.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link

Choose a reason for hiding this comment

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

Not done ;)

Copy link

Choose a reason for hiding this comment

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

Do we need this or not?

Copy link
Author

Choose a reason for hiding this comment

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

ERROR: ./test/extensions/filters/http/wasm/test_data/grpc_call_cpp.cc:21: unexpected direct dependency on google.protobuf, use the definitions in common/protobuf/protobuf.h instead.

The wasm .cc files disobey envoy style in this regard.

PiotrSikora
PiotrSikora approved these changes May 8, 2019
View reviewed changes
Copy link

@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.

LGTM, thanks!

3 small nits still left:

  • typo in include/envoy/grpc/async_client.h:187,
  • commented out dead code in source/extensions/common/wasm/wasm.cc:454,
  • unnecessary(?) change in tools/check_format.py:20.

include/envoy/grpc/async_client.h Outdated Show resolved Hide resolved
tools/check_format.py Outdated
@@ -16,7 +16,8 @@

EXCLUDED_PREFIXES = ("./generated/", "./thirdparty/", "./build", "./.git/", "./bazel-", "./.cache",
"./source/extensions/extensions_build_config.bzl",
"./tools/testdata/check_format/", "./tools/pyformat/", "./api/wasm/")
"./tools/testdata/check_format/", "./tools/pyformat/", "./api/wasm/",
"./test/extensions/filters/http/wasm/")
Copy link

Choose a reason for hiding this comment

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

Do we need this or not?

@jplevyak
Copy link
Author

jplevyak commented May 8, 2019

I changed the format exception to specifically be "./test/extensions/filters/http/wasm/test_data" so that it only applies to the WASM code.

I am going to merge this and we can talk about removing one of the gRPC Call APIs as a follow up.

@jplevyak jplevyak merged commit 34b0348 into istio:wasm May 8, 2019
@jplevyak jplevyak mentioned this pull request May 9, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bianpengyuan bianpengyuan bianpengyuan left review comments

@PiotrSikora PiotrSikora PiotrSikora approved these changes

@mandarjog mandarjog Awaiting requested review from mandarjog

Assignees

@bianpengyuan bianpengyuan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jplevyak @istio-testing @bianpengyuan @PiotrSikora