Skip to content

core: remove envoy_stream_options#689

Merged
rebello95 merged 7 commits intomasterfrom
stream_options
Feb 19, 2020
Merged

core: remove envoy_stream_options#689
rebello95 merged 7 commits intomasterfrom
stream_options

Conversation

@rebello95
Copy link
Contributor

As of #616, this is no longer used or necessary since it's handled by the router.

Resolves #643.

Signed-off-by: Michael Rebello me@michaelrebello.com

As of #616, this is no longer used or necessary since it's handled by the router.

Resolves #643.

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: Michael Rebello <me@michaelrebello.com>
@junr03
Copy link
Member

junr03 commented Feb 14, 2020

I had left this behind after I realized about #679. I was going to set the option up in this struct. Given @goaway took #679, up to him if he wants to use as is or do something else.

@rebello95
Copy link
Contributor Author

Leaving the struct makes sense to me if that's how we're going to set up HTTP/2 preferences per-stream (though a lot of the deletions in this PR will still be relevant). WDYT @goaway?

@goaway
Copy link
Contributor

goaway commented Feb 19, 2020

Honestly, I'm ambivalent. Headers can work for almost every potential use-case I can think of, and there's already precedent (retry policies, timeouts). The only downside of headers is that they aren't typed, but this is all internal anyways. This is a potentially useful feature for cases where there wouldn't be headers (redis connections?), but it's not like we can't revisit it when/if that's relevant.

I guess I'm (ever so) slightly in favor removing it for now, simply because it adds overhead to all the function signatures.

@rebello95
Copy link
Contributor Author

Sounds good. I agree that this could be used internally in the future, but we can always revert/revisit when that time comes (rather than keeping dead code on master).

@rebello95 rebello95 merged commit 2223fcc into master Feb 19, 2020
@rebello95 rebello95 deleted the stream_options branch February 19, 2020 02:05
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.

deprecated: delete envoy_stream_options

3 participants