config: reinstate #8478 (unification of delta and SotW xDS), reverted by #8939#8974
Conversation
|
There was a minor merge conflict with #8886 to resolve. While doing that I noticed that some comments in new_grpc_mux_impl.cc wrongly referred to "DeltaDiscoveryRequest" when they should say a generic "discovery request". I can undo that if you'd prefer it to be a pure revert revert. Other than the new_grpc_mux_impl.cc cleanup and the WellKnownGrpcStatus merge, this should be identical to #8478. |
…st would have caught it Signed-off-by: Fred Douglas <fredlas@google.com>
|
@fredlas can you check CI? @kyessenov once we get CI passing do you mind giving this branch a quick smoke test however you tested master last time? Thank you. /wait |
|
Sure, waiting for the CI to pass. |
Signed-off-by: Fred Douglas <fredlas@google.com>
…pcmux Signed-off-by: Fred Douglas <fredlas@google.com>
|
Done, updated some more GrpcStatuses. I'm also seeing that any _speed_test test is failing to build, due to a problem when trying to link with libpthread. I'm not sure if that's a newly uncovered quirk with just my own machine's environment, or a recently introduced actual breakage that hasn't yet been caught and fixed, but it's hard to imagine something in these changes causing it (and not causing it before). I guess we'll see once the tests get there. |
Signed-off-by: Fred Douglas <fredlas@google.com>
…pcmux Signed-off-by: Fred Douglas <fredlas@google.com>
|
@kyessenov looks good for a quick smoke test. Thank you! cc @wgallagher /wait-any |
|
I ran go-control-plane CI test, and it passes fine now. Thanks. |
|
Awesome, thanks. I'm relieved it got back in so quickly! |
|
@fredlas @mattklein123 hmm so apparently this change causes some of our instances to get stuck on startup -- unclear if it's CDS or SDS or something in XDS but something with XDS broke which prevents some of our binary/config configurations to start... Any ideas before I start carefully reading this PR? cc: @fishcakez |
|
@rgs1 if it's causing issues we will revert again, though a bit of debugging would be nice. Can you get any more info? |
|
@mattklein123 yeah, any ideas on what info would be useful? |
|
Logs? Which xDS? Etc.? If you can reliably reproduce any/all info would be helpful. |
|
It might have to wait until later/Mon; do if it already caused issues a
revert might be appreciated so that we can fetch some other stuff from
master. Thanks!
…On Fri, Nov 15, 2019, 6:38 PM Matt Klein ***@***.***> wrote:
Logs? Which xDS? Etc.? If you can reliably reproduce any/all info would be
helpful.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8974?email_source=notifications&email_token=AAKJSA6YTMRS3W4ICZKCKOLQT4JDRA5CNFSM4JL3GBSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEGZLFQ#issuecomment-554538390>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKJSA6T7FXSZRTTW6LBEU3QT4JDRANCNFSM4JL3GBSA>
.
|
|
Are you positive that if you revert the change the problem goes away? |
|
Positive -- tho honestly it's probably fine to wait until EOD Monday to
revert if we can't figure it out.
…On Fri, Nov 15, 2019, 6:41 PM Matt Klein ***@***.***> wrote:
Are you positive that if you revert the change the problem goes away?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8974?email_source=notifications&email_token=AAKJSAZL2ZZR7FD2VKQQ7R3QT4JPBA5CNFSM4JL3GBSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEGZSFY#issuecomment-554539287>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKJSA5GL2GL2UCW7IOJHITQT4JPBANCNFSM4JL3GBSA>
.
|
|
OK I think we should probably go ahead and start the revert to get master into a clean state and then we can debug in parallel, but I don't feel that strongly about it. |
…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 <mklein@lyft.com>
|
I opened #9044 and we can decide on Monday whether to merge or not. |
…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 <mklein@lyft.com>
4 participants
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, added regression test for #8927