Skip to content

kotlin: response interfaces#265

Merged
buildbreaker merged 13 commits intomasterfrom
demo-response-callback
Jul 22, 2019
Merged

kotlin: response interfaces#265
buildbreaker merged 13 commits intomasterfrom
demo-response-callback

Conversation

@buildbreaker
Copy link
Copy Markdown

@buildbreaker buildbreaker commented Jul 18, 2019 •
edited
Loading

Adding the Client interface and the streaming request interface method for opening a stream

Signed-off-by: Alan Chiu achiu@lyft.com

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

Description: Adding the Client and the streaming request method for opening a stream
Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

Demo response callback
fc80ad7 
Signed-off-by: Alan Chiu <achiu@lyft.com>
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Overall I think the response interfaces look great for an initial pass, but have a question about the StreamEmitter. This definitely paves the way for streaming support, and mostly maintains parity with the C interface while providing some helpful abstractions (such as parsing out the status code).

@rebello95 rebello95 mentioned this pull request Jul 18, 2019
Closed
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
more
7e1bef8 
Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker buildbreaker changed the title Demo response callback kotlin: response interfaces Jul 18, 2019
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
@rebello95
Copy link
Copy Markdown
Contributor

rebello95 commented Jul 18, 2019

Looks great to me after a couple comments!

@buildbreaker
Copy link
Copy Markdown
Author

buildbreaker commented Jul 18, 2019

I'm working on the comments/docs

Add docs
1930e52 
Signed-off-by: Alan Chiu <achiu@lyft.com>
@rebello95 rebello95 mentioned this pull request Jul 18, 2019
Closed
default to empty map
b6f58de 
Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker buildbreaker marked this pull request as ready for review July 18, 2019 23:53
@buildbreaker buildbreaker added this to the v0.2 "Primo" milestone Jul 18, 2019
rebello95
rebello95 reviewed Jul 18, 2019
View reviewed changes
rebello95
rebello95 reviewed Jul 19, 2019
View reviewed changes
Alan Chiu added 2 commits July 18, 2019 17:27
reverse
ee3f1a7 
Signed-off-by: Alan Chiu <achiu@lyft.com>
code
67f51f4 
Signed-off-by: Alan Chiu <achiu@lyft.com>
rebello95 added a commit that referenced this pull request Jul 19, 2019
@rebello95
swift: add library response interfaces
ebc3860 
Adds Swift interfaces for responses within Envoy. These should be at parity with the interfaces being added for Kotlin in #265.

Replaces #261.

Resolves #118.

Signed-off-by: Michael Rebello <mrebello@lyft.com>
@rebello95 rebello95 mentioned this pull request Jul 19, 2019
Merged
periods
11807a1 
Signed-off-by: Alan Chiu <achiu@lyft.com>
junr03
junr03 reviewed Jul 22, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

This is looking good. I left a few comments.

The comments apply to both PRs, just wanted to centralize discussion here given this is where we have left comments in the past.

Alan Chiu added 3 commits July 22, 2019 11:00
fixed stuff
c3abb41 
Signed-off-by: Alan Chiu <achiu@lyft.com>
more
68be06d 
Signed-off-by: Alan Chiu <achiu@lyft.com>
more
615c792 
Signed-off-by: Alan Chiu <achiu@lyft.com>
@rebello95 rebello95 mentioned this pull request Jul 22, 2019
Closed
goaway
goaway reviewed Jul 22, 2019
View reviewed changes
@goaway
Copy link
Copy Markdown
Contributor

goaway commented Jul 22, 2019

Looks great. I'm really liking the direction we've taken here, and I think this close mechanism is a clean way of allowing chaining while preventing the invalid, already-closed case.

@rebello95 this applies to your PR as well, though I'm posting comments here.

Alan Chiu added 2 commits July 22, 2019 12:40
errorCodeg
da51993 
Signed-off-by: Alan Chiu <achiu@lyft.com>
ResponseHandler
f444fbb 
Signed-off-by: Alan Chiu <achiu@lyft.com>
rebello95
rebello95 reviewed Jul 22, 2019
View reviewed changes
fix
6b16f9f 
Signed-off-by: Alan Chiu <achiu@lyft.com>
rebello95 added a commit that referenced this pull request Jul 22, 2019
@rebello95
swift: add library response interfaces (#273)
058bfb2 
Adds Swift interfaces for responses within Envoy. These should be at parity with the interfaces being added for Kotlin in #265.

Replaces #261.

Resolves #118.
Also resolves #247.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
goaway
goaway approved these changes Jul 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@goaway goaway left a comment

Choose a reason for hiding this comment

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

Nice work

@buildbreaker buildbreaker merged commit 8f4ec73 into master Jul 22, 2019
@buildbreaker buildbreaker deleted the demo-response-callback branch July 22, 2019 20:43
@buildbreaker
Copy link
Copy Markdown
Author

buildbreaker commented Jul 22, 2019

thanks @rebello95 @goaway !

jpsim pushed a commit to envoyproxy/envoy that referenced this pull request Nov 28, 2022
@rebello95 @jpsim
swift: add library response interfaces (#273)
829a93c 
Adds Swift interfaces for responses within Envoy. These should be at parity with the interfaces being added for Kotlin in envoyproxy/envoy-mobile#265.

Replaces envoyproxy/envoy-mobile#261.

Resolves envoyproxy/envoy-mobile#118.
Also resolves envoyproxy/envoy-mobile#247.

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
@rebello95 @jpsim
swift: add library response interfaces (#273)
3d1e1ad 
Adds Swift interfaces for responses within Envoy. These should be at parity with the interfaces being added for Kotlin in envoyproxy/envoy-mobile#265.

Replaces envoyproxy/envoy-mobile#261.

Resolves envoyproxy/envoy-mobile#118.
Also resolves envoyproxy/envoy-mobile#247.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goaway goaway goaway approved these changes

+2 more reviewers

@junr03 junr03 junr03 left review comments

@rebello95 rebello95 rebello95 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.2 "Primo"

Development

Successfully merging this pull request may close these issues.

4 participants

@buildbreaker @rebello95 @goaway @junr03