Skip to content

config: add GrpcStream: gRPC mechanisms pulled out of GrpcMux#5681

Merged
htuch merged 14 commits intoenvoyproxy:masterfrom
fredlas:REF_grpc_mux_impl
Jan 24, 2019
Merged

config: add GrpcStream: gRPC mechanisms pulled out of GrpcMux#5681
htuch merged 14 commits intoenvoyproxy:masterfrom
fredlas:REF_grpc_mux_impl

Conversation

@fredlas
Copy link
Copy Markdown
Contributor

@fredlas fredlas commented Jan 22, 2019

Description: Refactor necessary for #5466 to avoid completely duplicating this logic.
Risk Level: low
Testing: existing tests

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Copy link
Copy Markdown
Contributor Author

@fredlas fredlas left a comment

Choose a reason for hiding this comment

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

@jmarantz here is the refactor I mentioned, which you wanted to take a look at. @htuch this is a sort of sub-PR of #5466, to enable avoiding the code duplication you pointed out.

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Yep, this looks great, just some procedural comments mostly.

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@fredlas
Copy link
Copy Markdown
Contributor Author

fredlas commented Jan 23, 2019

I just realized that most of the names in this directory leave their association with xDS implicit, based on the directory they're living in. Would DiscoveryGrpcStream then make more sense as GrpcStream, despite sounding overly generic at first glance?

@htuch
Copy link
Copy Markdown
Member

htuch commented Jan 23, 2019

Since it's in the Config namespace, abbreviating is fine. Even XdsGrpcStream would be shorter :)

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@fredlas fredlas changed the title config: add DiscoveryGrpcStream: gRPC mechanisms pulled out of GrpcMux config: add GrpcStream: gRPC mechanisms pulled out of GrpcMux Jan 23, 2019
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Other than a nit, LGTM, thanks.

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks!

@htuch htuch merged commit 79dca84 into envoyproxy:master Jan 24, 2019
@fredlas fredlas deleted the REF_grpc_mux_impl branch January 24, 2019 19:45
@fredlas
Copy link
Copy Markdown
Contributor Author

fredlas commented Jan 28, 2019

Forgot to tag this as part of work on #4991.

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Jan 31, 2019
…roxy#5681)

Refactor necessary for envoyproxy#5466 to avoid completely duplicating this logic.

Risk Level: low
Testing: existing tests

Signed-off-by: Fred Douglas <fredlas@google.com>

Signed-off-by: Fred Douglas <43351173+fredlas@users.noreply.github.com>
fredlas added a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
…roxy#5681)

Refactor necessary for envoyproxy#5466 to avoid completely duplicating this logic.

Risk Level: low
Testing: existing tests

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
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.

3 participants