Merged
Conversation
|
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! |
rebello95
reviewed
Oct 15, 2019
Contributor
rebello95
left a comment
There was a problem hiding this comment.
Looks good overall! Some comments
library/kotlin/src/io/envoyproxy/envoymobile/EnvoyStreamEmitter.kt
Outdated
Show resolved
Hide resolved
library/kotlin/src/io/envoyproxy/envoymobile/GRPCResponseHandler.kt
Outdated
Show resolved
Hide resolved
library/kotlin/src/io/envoyproxy/envoymobile/GRPCResponseHandler.kt
Outdated
Show resolved
Hide resolved
library/kotlin/test/io/envoyproxy/envoymobile/GRPCResponseHandlerTest.kt
Outdated
Show resolved
Hide resolved
library/kotlin/test/io/envoyproxy/envoymobile/GRPCStreamEmitterTest.kt
Outdated
Show resolved
Hide resolved
library/kotlin/src/io/envoyproxy/envoymobile/GRPCStreamEmitter.kt
Outdated
Show resolved
Hide resolved
2f66ef2 to
6842746
Compare
added 15 commits
October 15, 2019 15:16
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
545a372 to
c492720
Compare
added 4 commits
October 15, 2019 15:19
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
rebello95
reviewed
Oct 16, 2019
library/kotlin/src/io/envoyproxy/envoymobile/GRPCResponseHandler.kt
Outdated
Show resolved
Hide resolved
rebello95
reviewed
Oct 16, 2019
library/kotlin/src/io/envoyproxy/envoymobile/GRPCResponseHandler.kt
Outdated
Show resolved
Hide resolved
rebello95
reviewed
Oct 16, 2019
rebello95
reviewed
Oct 16, 2019
library/kotlin/src/io/envoyproxy/envoymobile/GRPCResponseHandler.kt
Outdated
Show resolved
Hide resolved
rebello95
reviewed
Oct 16, 2019
rebello95
reviewed
Oct 16, 2019
rebello95
reviewed
Oct 16, 2019
rebello95
reviewed
Oct 16, 2019
rebello95
reviewed
Oct 16, 2019
Contributor
rebello95
left a comment
There was a problem hiding this comment.
Thanks for updating! Left another round of comments
rebello95
reviewed
Oct 16, 2019
library/kotlin/test/io/envoyproxy/envoymobile/GRPCStreamEmitterTest.kt
Outdated
Show resolved
Hide resolved
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 <me@michaelrebello.com>
added 2 commits
October 16, 2019 14:51
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: Alan Chiu <achiu@lyft.com>
rebello95
reviewed
Oct 16, 2019
library/kotlin/test/io/envoyproxy/envoymobile/GRPCStreamEmitterTest.kt
Outdated
Show resolved
Hide resolved
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 <me@michaelrebello.com>
added 2 commits
October 16, 2019 15:35
rebello95
reviewed
Oct 16, 2019
|
|
||
| val byteArray = bufferedStream.toByteArray() | ||
| val buffer = ByteBuffer.wrap(byteArray.sliceArray(1..4)) | ||
| buffer.order(ByteOrder.BIG_ENDIAN) |
Contributor
There was a problem hiding this comment.
Assuming the language does the right thing here and does the conversion to native format when you call .int below this should be fine
Author
There was a problem hiding this comment.
Yeah, it will write it in big endian here
rebello95
approved these changes
Oct 16, 2019
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 <me@michaelrebello.com>
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 <me@michaelrebello.com>
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 <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
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 <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
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 <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
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 <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
gRPC codec for kotlin
We are able to hit an Envoy backed gRPC server from
lyft.comWe 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 achiu@lyft.com
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:]