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

kotlin: grpc codec #472

Merged
merged 23 commits into from
Oct 17, 2019
Merged

kotlin: grpc codec #472

merged 23 commits into from
Oct 17, 2019

Conversation

buildbreaker
Copy link

@buildbreaker buildbreaker commented Sep 29, 2019

gRPC codec for kotlin

We are able to hit an Envoy backed gRPC server from lyft.com

We ran into some issues with testing (debug pull: #495). The first is outgoing ALPN is required for gRPC connections: #502. gRPC doesn't support disabling this option when starting up a service. We'll have to revisit this effort in the future.

For now, we are primarily missing #494 for local Envoy library e2e testing

Signed-off-by: Alan Chiu [email protected]

Description: gRPC codec for kotlin
Risk Level: low
Testing: unit/end-to-end
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

@stale
Copy link

stale bot commented Oct 6, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale label Oct 6, 2019
@buildbreaker buildbreaker removed the stale label Oct 9, 2019
@buildbreaker buildbreaker marked this pull request as ready for review October 15, 2019 16:38
Copy link
Contributor

@rebello95 rebello95 left a comment

Choose a reason for hiding this comment

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

Looks good overall! Some comments

Alan Chiu added 15 commits October 15, 2019 15:16
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
fix
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
fix
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Alan Chiu added 4 commits October 15, 2019 15:19
fix
Signed-off-by: Alan Chiu <[email protected]>
fix
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
codespell Outdated Show resolved Hide resolved
Copy link
Contributor

@rebello95 rebello95 left a comment

Choose a reason for hiding this comment

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

Thanks for updating! Left another round of comments

rebello95 added a commit that referenced this pull request Oct 16, 2019
Adding a test to match Android (see [this comment](#472 (comment))) to validate that gRPC streams are closed with `nil` trailers, so as to force the stream to close with a data frame per the gRPC protocol spec.

Signed-off-by: Michael Rebello <[email protected]>
Alan Chiu added 2 commits October 16, 2019 14:51
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>
rebello95 added a commit that referenced this pull request Oct 16, 2019
Adding a test to match Android (see [this comment](#472 (comment))) to validate that gRPC streams are closed with `nil` trailers, so as to force the stream to close with a data frame per the gRPC protocol spec.

Signed-off-by: Michael Rebello <[email protected]>
Alan Chiu added 2 commits October 16, 2019 15:35
Signed-off-by: Alan Chiu <[email protected]>
Signed-off-by: Alan Chiu <[email protected]>

val byteArray = bufferedStream.toByteArray()
val buffer = ByteBuffer.wrap(byteArray.sliceArray(1..4))
buffer.order(ByteOrder.BIG_ENDIAN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming the language does the right thing here and does the conversion to native format when you call .int below this should be fine

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it will write it in big endian here

@buildbreaker buildbreaker merged commit 1982e78 into master Oct 17, 2019
@buildbreaker buildbreaker deleted the kotlin-grpc-codec branch October 17, 2019 01:23
rebello95 added a commit that referenced this pull request Oct 17, 2019
Updates the Swift gRPC response compression error handling logic to match Android's:
- Instead of silently failing, call `onError` in a terminal state and clear out other callbacks
- Retain the `GRPCResponseHandler` while the core Envoy engine retains the underlying `onData` callback to ensure that error and message propogation can be called by the handler

Context: #472 (comment)

Signed-off-by: Michael Rebello <[email protected]>
rebello95 added a commit that referenced this pull request Oct 17, 2019
Updates the Swift gRPC response compression error handling logic to match Android's:
- Instead of silently failing, call `onError` in a terminal state and clear out other callbacks
- Retain the `GRPCResponseHandler` while the core Envoy engine retains the underlying `onData` callback to ensure that error and message propogation can be called by the handler

Context: #472 (comment)

Signed-off-by: Michael Rebello <[email protected]>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 28, 2022
Adding a test to match Android (see [this comment](envoyproxy/envoy-mobile#472 (comment))) to validate that gRPC streams are closed with `nil` trailers, so as to force the stream to close with a data frame per the gRPC protocol spec.

Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 28, 2022
Updates the Swift gRPC response compression error handling logic to match Android's:
- Instead of silently failing, call `onError` in a terminal state and clear out other callbacks
- Retain the `GRPCResponseHandler` while the core Envoy engine retains the underlying `onData` callback to ensure that error and message propogation can be called by the handler

Context: envoyproxy/envoy-mobile#472 (comment)

Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
Adding a test to match Android (see [this comment](envoyproxy/envoy-mobile#472 (comment))) to validate that gRPC streams are closed with `nil` trailers, so as to force the stream to close with a data frame per the gRPC protocol spec.

Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 29, 2022
Updates the Swift gRPC response compression error handling logic to match Android's:
- Instead of silently failing, call `onError` in a terminal state and clear out other callbacks
- Retain the `GRPCResponseHandler` while the core Envoy engine retains the underlying `onData` callback to ensure that error and message propogation can be called by the handler

Context: envoyproxy/envoy-mobile#472 (comment)

Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: JP Simard <[email protected]>
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.

2 participants