Skip to content

core: expose stream options#456

Closed
junr03 wants to merge 1 commit intomasterfrom
buffering
Closed

core: expose stream options#456
junr03 wants to merge 1 commit intomasterfrom
buffering

Conversation

@junr03
Copy link
Copy Markdown
Member

@junr03 junr03 commented Sep 19, 2019

Description: this PR introduces envoy_stream_options which is a subset of AsyncClient::StreamOptions, that can be used in the bridge layer.
Risk Level: med - new functionality
Testing: unit tests missing, will update after initial discussion

Signed-off-by: Jose Nino jnino@lyft.com

Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Copy Markdown
Member Author

junr03 commented Sep 19, 2019

@buildbreaker @rebello95 @goaway this PR exposes stream data buffering in order to allow for retries. I only did the work here to expose the option and wire it all the way to the underlying envoy stream.

We have an open question on how we want the platform code to deal with turning on buffering, and its relation with retries. Therefore, the way I have implemented here leaves us open for several possibilities.

@rebello95 rebello95 requested a review from goaway September 20, 2019 01:05
@stale
Copy link
Copy Markdown

stale bot commented Sep 27, 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 Sep 27, 2019
@stale
Copy link
Copy Markdown

stale bot commented Oct 4, 2019

This pull request has been automatically closed because it has not had activity in the last 14 days. 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 closed this Oct 4, 2019
@junr03 junr03 reopened this Oct 7, 2019
@stale stale bot removed the stale label Oct 7, 2019
@junr03
Copy link
Copy Markdown
Member Author

junr03 commented Oct 7, 2019

re-opening now that I am back. @goaway do you mind taking a look and if it looks good on first pass I can rebase and we can review?

@stale
Copy link
Copy Markdown

stale bot commented Oct 14, 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 14, 2019
@junr03
Copy link
Copy Markdown
Member Author

junr03 commented Oct 16, 2019

closing this in favor of: #511

@junr03 junr03 closed this Oct 16, 2019
junr03 added a commit that referenced this pull request Oct 18, 2019
Description: after a bit more discussion we have decided to go ahead and make buffering configurable. This PR revives #456
Risk Level: med - adds new surface area to the API
Testing: local apps and CI

Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03 junr03 deleted the buffering branch November 13, 2019 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant