Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: unification of delta and SotW gRPC xDS #8478

Merged
merged 66 commits into from
Nov 6, 2019

Conversation

fredlas
Copy link
Contributor

@fredlas fredlas commented Oct 3, 2019

Delta and state-of-the-world gRPC xDS now share the same GrpcMux and
Subscription implementations. These are the implementations added in #7293 for
delta. These implementations are a rewrite/extensive refactoring of the old
GrpcMuxImpl (deleted by this PR) and associated logic.

Risk Level: high (rewrite/large refactor of gRPC xDS)
Testing: added sotw_subscription_state_test

Signed-off-by: Fred Douglas <[email protected]>
Signed-off-by: Fred Douglas <[email protected]>
Signed-off-by: Fred Douglas <[email protected]>
Signed-off-by: Fred Douglas <[email protected]>
Signed-off-by: Fred Douglas <[email protected]>
Signed-off-by: Fred Douglas <[email protected]>
@fredlas
Copy link
Contributor Author

fredlas commented Nov 6, 2019

@mattklein123 @wgallagher done! test/... passed locally for me, and it looks like the CI tests have now passed too, other than a bit of infrastructure breakage.

@wgallagher
Copy link

awesome!

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -9,39 +9,51 @@ you can mostly forget the filesystem/REST/gRPC distinction, and you can
especially forget about the gRPC flavors. All of that is specified in the
bootstrap config, which is read and put into action by ClusterManagerImpl.

Note that there can be multiple active gRPC subscriptions for a single resource
Copy link
Member

Choose a reason for hiding this comment

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

@wgallagher in a follow up change can we move this doc to source/docs where our dev docs go and/or figure out if this should be merged into the RST docs?

@@ -9,52 +9,62 @@
#include "common/config/delta_subscription_state.h"
Copy link
Member

Choose a reason for hiding this comment

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

@wgallagher I assume eventually we are going to rename files away from "new" if we only have a single implementation? Not sure of the history here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, totally. This PR keeps it as new to minimize churn, and most importantly to allow the github diffs in this PR to actually be meaningful for those files.

@repokitteh-read-only repokitteh-read-only bot removed the api label Nov 6, 2019
@mattklein123 mattklein123 merged commit 443bc33 into envoyproxy:master Nov 6, 2019
@fredlas
Copy link
Contributor Author

fredlas commented Nov 6, 2019

Wooo, thanks! Almost fully clean now. As you mentioned, the "new" in NewGrpcMuxImpl is temporary - I'll be sending out a PR for that shortly.

@wgallagher
Copy link

I'd be happy to make the name change. Would be a good first envoy PR.

@fredlas
Copy link
Contributor Author

fredlas commented Nov 6, 2019

Awesome, thanks, go for it!

mattklein123 added a commit that referenced this pull request Nov 7, 2019
mattklein123 added a commit that referenced this pull request Nov 8, 2019
mattklein123 pushed a commit that referenced this pull request Nov 12, 2019
mattklein123 added a commit that referenced this pull request Nov 15, 2019
…e_names_ (#8918)"

This reverts commit 80aedc1.

Revert "config: rename NewGrpcMuxImpl -> GrpcMuxImpl (#8919)"
This reverts commit 6d50553.

Revert "config: reinstate #8478 (unification of delta and SotW xDS), reverted by #8939 (#8974)"
This reverts commit a37522c.

Signed-off-by: Matt Klein <[email protected]>
mattklein123 added a commit that referenced this pull request Nov 16, 2019
…e_names_ (#8918)" (#9044)

This reverts commit 80aedc1.

Revert "config: rename NewGrpcMuxImpl -> GrpcMuxImpl (#8919)"
This reverts commit 6d50553.

Revert "config: reinstate #8478 (unification of delta and SotW xDS), reverted by #8939 (#8974)"
This reverts commit a37522c.

Signed-off-by: Matt Klein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants